On Tue, Nov 15, 2022 at 02:43:06PM +0400, Pavel Borisov wrote:
> I've looked through the patch again. I agree it looks better and can
> be committed.
> Mark it as RfC now.
Okay, applied, then. The SQL function names speak by themselves, even
if some of them refer to pages but they act on
On Tue, Nov 15, 2022 at 11:39:20AM +0100, Daniel Gustafsson wrote:
> + /* write given data to the page */
> + strncpy(TestSlruCtl->shared->page_buffer[slotno], data, BLCKSZ - 1);
>
> Would it make sense to instead use pg_pwrite to closer match the code being
> tested?
Hmm. I am not
Hi, Alexander!
> > I have reworked that as per the attached, that provides basically the
> > same coverage, going through a SQL interface for the whole thing.
> > Like all the other tests of its kind, this does not use a TAP test,
> > relying on a custom configuration file instead. This still
> On 15 Nov 2022, at 11:15, Aleksander Alekseev
> wrote:
>> What do you think?
>
> It looks much better than before. I replaced strcpy() with strncpy()
> and pgindent'ed the code.
+ /* write given data to the page */
+ strncpy(TestSlruCtl->shared->page_buffer[slotno], data, BLCKSZ
Hi Michael,
> I have reworked that as per the attached, that provides basically the
> same coverage, going through a SQL interface for the whole thing.
> Like all the other tests of its kind, this does not use a TAP test,
> relying on a custom configuration file instead. This still needs some
>
r
Date: Mon, 14 Nov 2022 20:16:36 +0900
Subject: [PATCH v6] Add unit tests for SLRU
SimpleLruInit() checks whether it is called under postmaster. For this reason
the tests can't be implemented in regress.c without changing the interface.
We wanted to avoid this. As a result the tests were
On Thu, Nov 10, 2022 at 06:40:44PM +0300, Aleksander Alekseev wrote:
> Fair enough. PFA the corrected patch v5.
Is there a reason why you need a TAP test here? It is by design more
expensive than pg_regress and it does not require --enable-tap-tests.
See for example what we do for
Hi Michael,
Thanks for the review and also for committing 5ca3645.
> This patch redesigns SimpleLruInit() [...]
> I am not really convinced that this is something that a
> patch aimed at extending testing coverage should redesign, especially
> with a routine as old as that.
> [...] you'd better
On Thu, Nov 10, 2022 at 04:01:02PM +0900, Michael Paquier wrote:
> I have looked at what you have here..
The comment at the top of SimpleLruInit() for sync_handler has been
fixed as of 5ca3645, and backpatched. Nice catch.
--
Michael
signature.asc
Description: PGP signature
On Wed, Apr 13, 2022 at 03:51:30PM +0400, Pavel Borisov wrote:
> Only one thing to note. Maybe it would be good not to copy-paste Assert
> after every call of SimpleLruInit, putting it into the wrapper function
> instead. So the test can call calling the inner function (without assert)
> and all
>
> Hi hackers,
>
> > Here is version 3 of the patch.
> > [...]
> > I think the tests are about as good as they will ever get.
>
> Here is version 4. Same as v3, but with resolved conflicts against the
> current `master` branch.
>
Hi, Alexander!
The test seems good enough to be pushed.
Only one
Hi hackers,
> Here is version 3 of the patch.
> [...]
> I think the tests are about as good as they will ever get.
Here is version 4. Same as v3, but with resolved conflicts against the
current `master` branch.
--
Best regards,
Aleksander Alekseev
v4-0001-Unit-tests-for-SLRU.patch
Hi hackers,
> OK, here is an updated version of the patch. Changes comparing to v1:
>
> * Tests are moved to regress.c, as asked by the majority;
> * SimpleLruInit() returns void as before, per Daniel's feedback;
> * Most of the initial refactorings were reverted in order to keep the patch
> as
Hi hackers,
>> Again, I don't have a strong opinion here. If you insist, I will place the
>> tests to regress.c.
>
> It is up to committer to decide, but I think it would be good to place tests
> in regression. In my opinion, many things from core may be used by extensions.
> And then it is up to
>
> Again, I don't have a strong opinion here. If you insist, I will place the
> tests to regress.c.
>
It is up to committer to decide, but I think it would be good to place
tests in regression.
In my opinion, many things from core may be used by extensions. And then it
is up to extension authors
Hi Daniel,
> It also doesn't seem all that appealing that SimpleLruInit can
> return false on successful function invocation, it makes for a confusing
API.
Agree. I think using an additional `bool *found` argument would be less
confusing.
Noah, Pavel,
>> The default place for this kind of test
пт, 1 апр. 2022 г. в 07:47, Noah Misch :
> On Thu, Mar 31, 2022 at 05:30:41PM +0300, Aleksander Alekseev wrote:
> > I used src/test/modules/test_* modules as an example.
>
> > If time permits, please take a quick look at the patch and let me know
> > if I'm moving the right direction. There will
On Thu, Mar 31, 2022 at 05:30:41PM +0300, Aleksander Alekseev wrote:
> I used src/test/modules/test_* modules as an example.
> If time permits, please take a quick look at the patch and let me know
> if I'm moving the right direction. There will be more tests in the
> final version, but I would
> On 31 Mar 2022, at 16:30, Aleksander Alekseev
> wrote:
Thanks for hacking on increasing test coverage!
> While on it, I moved the Asserts() outside of SimpleLruInit(). It didn't seem
> to be a right place to check the IsUnderPostmaster value, and complicated the
> test implementation.
+ *
Hi hackers,
I learned from Peter [1] that SLRU test coverage leaves much to be
desired and makes it difficult to refactor it. Here is a draft of a
patch that tries to address it.
I used src/test/modules/test_* modules as an example. While on it, I
moved the Asserts() outside of SimpleLruInit().
20 matches
Mail list logo
