Re: 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.

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.