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&gt;.
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 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.

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&gt;.
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 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;