Обсуждение: Refactoring of pg_resetwal/t/001_basic.pl

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.

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.

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.

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

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
anddocs, 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;