Horton
Menu

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. pg_resetwal: Add more tests and test coverage

  1. Refactoring of pg_resetwal/t/001_basic.pl

    Maxim Orlov <orlovmg@gmail.com> — 2024-03-21T16:58:28Z

    Hi!

In commit 7b5275eec more tests and test coverage were added into
pg_resetwal/t/001_basic.pl.
All the added stuff are pretty useful in my view.  Unfortunately, there
were some magic constants
been used.  In overall, this is not a problem.  But while working on 64 bit
XIDs I've noticed these
changes and spent some time to figure it out what this magic values are
stands fore.

And it turns out that I’m not the only one.

So, by Svetlana Derevyanko's suggestion, I made this patch.  I add
constants, just like we did
in verify_heapam tests.

Sidenote here: in defines in multixact.c TransactionId type used, but I'm
sure this is not correct,
since we're dealing here with MultiXactId and MultiXactOffset.  For now,
this is obviously not a
problem, since sizes of this particular types are equal.  But this will
manifest itself when we switch
to the 64 bits types for MultiXactOffset or MultiXactId.

As always, reviews and opinions are very welcome!

-- 
Best regards,
Maxim Orlov.

  2. Re: Refactoring of pg_resetwal/t/001_basic.pl

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-21T22:08:10Z

    On 21.03.24 17:58, Maxim Orlov wrote:
> In commit 7b5275eec more tests and test coverage were added into 
> pg_resetwal/t/001_basic.pl <http://001_basic.pl>.
> All the added stuff are pretty useful in my view.  Unfortunately, there 
> were some magic constants
> been used.  In overall, this is not a problem.  But while working on 64 
> bit XIDs I've noticed these
> changes and spent some time to figure it out what this magic values are 
> stands fore.
> 
> And it turns out that I’m not the only one.
> 
> So, by Svetlana Derevyanko's suggestion, I made this patch.  I add 
> constants, just like we did
> in verify_heapam tests.

Ok, this sounds like a reasonable idea.

> 
> Sidenote here: in defines in multixact.c TransactionId type used, but 
> I'm sure this is not correct,
> since we're dealing here with MultiXactId and MultiXactOffset.  For now, 
> this is obviously not a
> problem, since sizes of this particular types are equal.  But this will 
> manifest itself when we switch
> to the 64 bits types for MultiXactOffset or MultiXactId.

Please send a separate patch for this if you want to propose any changes.

  3. Re: Refactoring of pg_resetwal/t/001_basic.pl

    Maxim Orlov <orlovmg@gmail.com> — 2024-03-22T10:27:23Z

    On Fri, 22 Mar 2024 at 01:08, Peter Eisentraut <peter@eisentraut.org> wrote:

> Please send a separate patch for this if you want to propose any changes.
>
>
Thank you for your reply.  Here is the second one.  I've change types and
argument
names for the macro functions, so that they better reflect the reality.

-- 
Best regards,
Maxim Orlov.

  4. Re: Refactoring of pg_resetwal/t/001_basic.pl

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-25T14:10:47Z

    On 21.03.24 17:58, Maxim Orlov wrote:
> In commit 7b5275eec more tests and test coverage were added into 
> pg_resetwal/t/001_basic.pl <http://001_basic.pl>.
> All the added stuff are pretty useful in my view.  Unfortunately, there 
> were some magic constants
> been used.  In overall, this is not a problem.  But while working on 64 
> bit XIDs I've noticed these
> changes and spent some time to figure it out what this magic values are 
> stands fore.
> 
> And it turns out that I’m not the only one.
> 
> So, by Svetlana Derevyanko's suggestion, I made this patch.  I add 
> constants, just like we did
> in verify_heapam tests.

Consider this change:

-$mult = 32 * $blcksz / 4;
+$mult = SLRU_PAGES_PER_SEGMENT * $blcksz / MXOFF_SIZE;

with

+use constant SLRU_PAGES_PER_SEGMENT => 32;
+use constant MXOFF_SIZE => 4;

SLRU_PAGES_PER_SEGMENT is a constant that also exists in the source 
code, so good.

But MXOFF_SIZE doesn't exist anywhere else.  The actual formula uses
sizeof(MultiXactOffset), which isn't obvious from your patch.  So this 
just moves the magic constants around by one level.

The TAP test says

# these use the guidance from the documentation

and the documentation in this case says

SLRU_PAGES_PER_SEGMENT * BLCKSZ / sizeof(MultiXactOffset)

I think if we're going to add more symbols, then it has to be done 
consistently in the source code, the documentation, and the tests, not 
just one of them.

  5. Re: Refactoring of pg_resetwal/t/001_basic.pl

    Svetlana Derevyanko <s.derevyanko@postgrespro.ru> — 2024-03-26T11:53:35Z

    Peter Eisentraut писал(а) 2024-03-25 17:10:

> But MXOFF_SIZE doesn't exist anywhere else.  The actual formula uses
> sizeof(MultiXactOffset), which isn't obvious from your patch.  So this 
> just moves the magic constants around by one level.
> 
> I think if we're going to add more symbols, then it has to be done 
> consistently in the source code, the documentation, and the tests, not 
> just one of them.
> 

Hello!
Thank you for your reply.

Attached is the updated version of patch for pg_resetwal test. I added 
definitions for MXOFF_SIZE and MXID_SIZE constants in multixact.c (and 
replaced use of sizeof(MultiXactId) and sizeof(MultiXactOffset) 
accordingly). Also changed multipliers for pg_xact/members/offset on 
CLOG_XACTS_PER_PAGE/MULTIXACT_MEMBERS_PER_PAGE/MULTIXACT_OFFSETS_PER_PAGE 
both in src/bin/pg_resetwal/t/001_basic.pl and docs, since it seems to 
me that this makes things more clear.

What do you think?


Best regards,
Svetlana Derevyanko.

  6. Re: Refactoring of pg_resetwal/t/001_basic.pl

    Michael Paquier <michael@paquier.xyz> — 2024-04-04T01:58:48Z

    On Tue, Mar 26, 2024 at 02:53:35PM +0300, Svetlana Derevyanko wrote:
> What do you think?
>
> +use constant SLRU_PAGES_PER_SEGMENT => 32;

Well, I disagree with what you are doing here, adding a hardcoded
dependency between the test code and the backend code.  I would
suggest to use a more dynamic approach and retrieve such values
directly from the headers.  See scan_server_header() in
039_end_of_wal.pl as one example.  7b5275eec3a5 is newer than
bae868caf222, so the original commit could have used that, as well.
--
Michael

  7. Re: Refactoring of pg_resetwal/t/001_basic.pl

    Peter Eisentraut <peter@eisentraut.org> — 2024-04-04T09:10:11Z

    On 26.03.24 12:53, Svetlana Derevyanko wrote:
> Peter Eisentraut писал(а) 2024-03-25 17:10:
> 
>> But MXOFF_SIZE doesn't exist anywhere else.  The actual formula uses
>> sizeof(MultiXactOffset), which isn't obvious from your patch.  So this 
>> just moves the magic constants around by one level.
>>
>> I think if we're going to add more symbols, then it has to be done 
>> consistently in the source code, the documentation, and the tests, not 
>> just one of them.
>>
> 
> Hello!
> Thank you for your reply.
> 
> Attached is the updated version of patch for pg_resetwal test. I added 
> definitions for MXOFF_SIZE and MXID_SIZE constants in multixact.c (and 
> replaced use of sizeof(MultiXactId) and sizeof(MultiXactOffset) 
> accordingly). Also changed multipliers for pg_xact/members/offset on 
> CLOG_XACTS_PER_PAGE/MULTIXACT_MEMBERS_PER_PAGE/MULTIXACT_OFFSETS_PER_PAGE both in src/bin/pg_resetwal/t/001_basic.pl and docs, since it seems to me that this makes things more clear.
> 
> What do you think?

I don't know.  This patch does not fill me with joy.  These additional 
defines ultimately make the code itself harder to comprehend.

Maybe the original request could be satisfied by adding more comments to 
the test files, like

  @files = get_slru_files('pg_xact');
+# SLRU_PAGES_PER_SEGMENT * BLCKSZ * CLOG_XACTS_PER_BYTE
  $mult = 32 * $blcksz * 4;