Refactoring of pg_resetwal/t/001_basic.pl
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.
Attachments:
v1-0001-Refactor-pg_resetwal-t-001_basic.pl.patchapplication/octet-stream; name=v1-0001-Refactor-pg_resetwal-t-001_basic.pl.patchDownload
From 65c211694f31b248e0a5f5fcf499f82fad72574c Mon Sep 17 00:00:00 2001
From: Maxim Orlov <m.orlov@postgrespro.ru>
Date: Thu, 21 Mar 2024 19:32:54 +0300
Subject: [PATCH v1] Refactor pg_resetwal/t/001_basic.pl
Use constants instead of magic numbers
Per Svetlana Derevyanko's suggestion.
Author: Maxim Orlov <orlovmg@gmail.com>
---
src/bin/pg_resetwal/t/001_basic.pl | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index 9829e48106..4cb1f5c374 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -205,8 +205,16 @@ push @cmd,
'-c',
sprintf("%d,%d", hex($files[0]) == 0 ? 3 : hex($files[0]), hex($files[-1]));
+use constant SLRU_PAGES_PER_SEGMENT => 32;
+use constant MXOFF_SIZE => 4;
+use constant MXID_SIZE => 4;
+use constant MULTIXACT_MEMBERS_PER_MEMBERGROUP => 4;
+use constant MULTIXACT_FLAGBYTES_PER_GROUP => 4;
+use constant MULTIXACT_MEMBERGROUP_SIZE => MXID_SIZE * MULTIXACT_MEMBERS_PER_MEMBERGROUP + MULTIXACT_FLAGBYTES_PER_GROUP;
+use constant CLOG_XACTS_PER_BYTE => 4;
+
@files = get_slru_files('pg_multixact/offsets');
-$mult = 32 * $blcksz / 4;
+$mult = SLRU_PAGES_PER_SEGMENT * $blcksz / MXOFF_SIZE;
# -m argument is "new,old"
push @cmd, '-m',
sprintf("%d,%d",
@@ -214,11 +222,11 @@ push @cmd, '-m',
hex($files[0]) == 0 ? 1 : hex($files[0] * $mult));
@files = get_slru_files('pg_multixact/members');
-$mult = 32 * int($blcksz / 20) * 4;
+$mult = SLRU_PAGES_PER_SEGMENT * int($blcksz / MULTIXACT_MEMBERGROUP_SIZE) * MXID_SIZE;
push @cmd, '-O', (hex($files[-1]) + 1) * $mult;
@files = get_slru_files('pg_xact');
-$mult = 32 * $blcksz * 4;
+$mult = SLRU_PAGES_PER_SEGMENT * $blcksz * CLOG_XACTS_PER_BYTE;
push @cmd,
'-u', (hex($files[0]) == 0 ? 3 : hex($files[0]) * $mult),
'-x', ((hex($files[-1]) + 1) * $mult);
--
2.44.0
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 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.
Attachments:
v1-0002-Use-proper-types-in-defines-in-multixact.c.patchapplication/octet-stream; name=v1-0002-Use-proper-types-in-defines-in-multixact.c.patchDownload
From ac587fc638a1d1f0bc75f8cc64201e92fe4f200c Mon Sep 17 00:00:00 2001
From: Maxim Orlov <orlovmg@gmail.com>
Date: Fri, 22 Mar 2024 11:54:10 +0300
Subject: [PATCH v1 2/2] Use proper types in defines in multixact.c
Use appropriate type casts to MultiXactId and MultiXactOffset instead of
TransactionId in macro functions and defines.
Author: Maxim Orlov <orlovmg@gmail.com>
---
src/backend/access/transam/multixact.c | 28 +++++++++++++-------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 83b578dced..e4a5924911 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -136,7 +136,7 @@
(MULTIXACT_FLAGBYTES_PER_GROUP * MXACT_MEMBER_FLAGS_PER_BYTE)
/* size in bytes of a complete group */
#define MULTIXACT_MEMBERGROUP_SIZE \
- (sizeof(TransactionId) * MULTIXACT_MEMBERS_PER_MEMBERGROUP + MULTIXACT_FLAGBYTES_PER_GROUP)
+ (sizeof(MultiXactId) * MULTIXACT_MEMBERS_PER_MEMBERGROUP + MULTIXACT_FLAGBYTES_PER_GROUP)
#define MULTIXACT_MEMBERGROUPS_PER_PAGE (BLCKSZ / MULTIXACT_MEMBERGROUP_SIZE)
#define MULTIXACT_MEMBERS_PER_PAGE \
(MULTIXACT_MEMBERGROUPS_PER_PAGE * MULTIXACT_MEMBERS_PER_MEMBERGROUP)
@@ -155,30 +155,30 @@
((uint32) ((0xFFFFFFFF % MULTIXACT_MEMBERS_PER_PAGE) + 1))
/* page in which a member is to be found */
-#define MXOffsetToMemberPage(xid) ((xid) / (TransactionId) MULTIXACT_MEMBERS_PER_PAGE)
-#define MXOffsetToMemberSegment(xid) (MXOffsetToMemberPage(xid) / SLRU_PAGES_PER_SEGMENT)
+#define MXOffsetToMemberPage(off) ((off) / (MultiXactOffset) MULTIXACT_MEMBERS_PER_PAGE)
+#define MXOffsetToMemberSegment(off) (MXOffsetToMemberPage(off) / SLRU_PAGES_PER_SEGMENT)
/* Location (byte offset within page) of flag word for a given member */
-#define MXOffsetToFlagsOffset(xid) \
- ((((xid) / (TransactionId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) % \
- (TransactionId) MULTIXACT_MEMBERGROUPS_PER_PAGE) * \
- (TransactionId) MULTIXACT_MEMBERGROUP_SIZE)
-#define MXOffsetToFlagsBitShift(xid) \
- (((xid) % (TransactionId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) * \
+#define MXOffsetToFlagsOffset(off) \
+ ((((off) / (MultiXactId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) % \
+ (MultiXactId) MULTIXACT_MEMBERGROUPS_PER_PAGE) * \
+ (MultiXactId) MULTIXACT_MEMBERGROUP_SIZE)
+#define MXOffsetToFlagsBitShift(off) \
+ (((off) % (MultiXactId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) * \
MXACT_MEMBER_BITS_PER_XACT)
/* Location (byte offset within page) of TransactionId of given member */
-#define MXOffsetToMemberOffset(xid) \
- (MXOffsetToFlagsOffset(xid) + MULTIXACT_FLAGBYTES_PER_GROUP + \
- ((xid) % MULTIXACT_MEMBERS_PER_MEMBERGROUP) * sizeof(TransactionId))
+#define MXOffsetToMemberOffset(off) \
+ (MXOffsetToFlagsOffset(off) + MULTIXACT_FLAGBYTES_PER_GROUP + \
+ ((off) % MULTIXACT_MEMBERS_PER_MEMBERGROUP) * sizeof(TransactionId))
/* Multixact members wraparound thresholds. */
#define MULTIXACT_MEMBER_SAFE_THRESHOLD (MaxMultiXactOffset / 2)
#define MULTIXACT_MEMBER_DANGER_THRESHOLD \
(MaxMultiXactOffset - MaxMultiXactOffset / 4)
-#define PreviousMultiXactId(xid) \
- ((xid) == FirstMultiXactId ? MaxMultiXactId : (xid) - 1)
+#define PreviousMultiXactId(mxid) \
+ ((mxid) == FirstMultiXactId ? MaxMultiXactId : (mxid) - 1)
/*
* Links to shared-memory data structures for MultiXact control
--
2.44.0
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.
Attachments:
v2-0001-Refactor-pg_resetwal-t-001_basic.pl.patchtext/x-diff; name=v2-0001-Refactor-pg_resetwal-t-001_basic.pl.patchDownload
From a1bbaddbc00004e97e95ed15b51e48386be5fb4b Mon Sep 17 00:00:00 2001
From: Svetlana Derevyanko <s.derevyanko@postgrespro.ru>
Date: Tue, 26 Mar 2024 13:09:43 +0300
Subject: [PATCH v2] Refactor pg_resetwal/t/001_basic.pl
Use constants instead of magic numbers.
Added MXID_SIZE and MXOFF_SIZE constants.
Changed desciptions for pg_resetwal options in docs.
---
doc/src/sgml/ref/pg_resetwal.sgml | 8 ++++----
src/backend/access/transam/multixact.c | 15 +++++++++------
src/bin/pg_resetwal/t/001_basic.pl | 18 +++++++++++++++---
3 files changed, 28 insertions(+), 13 deletions(-)
diff --git a/doc/src/sgml/ref/pg_resetwal.sgml b/doc/src/sgml/ref/pg_resetwal.sgml
index cf9c7e70f2..d8de5e2e29 100644
--- a/doc/src/sgml/ref/pg_resetwal.sgml
+++ b/doc/src/sgml/ref/pg_resetwal.sgml
@@ -274,7 +274,7 @@ PostgreSQL documentation
names are in hexadecimal, so the easiest way to do this is to specify
the option value in hexadecimal and append four zeroes.
</para>
- <!-- 65536 = SLRU_PAGES_PER_SEGMENT * BLCKSZ / sizeof(MultiXactOffset) -->
+ <!-- 65536 = SLRU_PAGES_PER_SEGMENT * MULTIXACT_OFFSETS_PER_PAGE -->
</listitem>
</varlistentry>
@@ -309,7 +309,7 @@ PostgreSQL documentation
The file names are in hexadecimal. There is no simple recipe such as
the ones for other options of appending zeroes.
</para>
- <!-- 52352 = SLRU_PAGES_PER_SEGMENT * floor(BLCKSZ/20) * 4; see multixact.c -->
+ <!-- 52352 = SLRU_PAGES_PER_SEGMENT * MULTIXACT_MEMBERS_PER_PAGE; see multixact.c -->
</listitem>
</varlistentry>
@@ -358,7 +358,7 @@ PostgreSQL documentation
in <filename>pg_xact</filename>, <literal>-u 0x700000</literal> will work (five
trailing zeroes provide the proper multiplier).
</para>
- <!-- 1048576 = SLRU_PAGES_PER_SEGMENT * BLCKSZ * CLOG_XACTS_PER_BYTE -->
+ <!-- 1048576 = SLRU_PAGES_PER_SEGMENT * CLOG_XACTS_PER_PAGE -->
</listitem>
</varlistentry>
@@ -380,7 +380,7 @@ PostgreSQL documentation
in <filename>pg_xact</filename>, <literal>-x 0x1200000</literal> will work (five
trailing zeroes provide the proper multiplier).
</para>
- <!-- 1048576 = SLRU_PAGES_PER_SEGMENT * BLCKSZ * CLOG_XACTS_PER_BYTE -->
+ <!-- 1048576 = SLRU_PAGES_PER_SEGMENT * CLOG_XACTS_PER_PAGE -->
</listitem>
</varlistentry>
</variablelist>
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 83b578dced..6bda000120 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -104,8 +104,11 @@
* MultiXactOffsetPagePrecedes).
*/
+#define MXOFF_SIZE sizeof(MultiXactOffset)
+#define MXID_SIZE sizeof(MultiXactId)
+
/* We need four bytes per offset */
-#define MULTIXACT_OFFSETS_PER_PAGE (BLCKSZ / sizeof(MultiXactOffset))
+#define MULTIXACT_OFFSETS_PER_PAGE (BLCKSZ / MXOFF_SIZE)
#define MultiXactIdToOffsetPage(xid) \
((xid) / (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
@@ -1765,7 +1768,7 @@ AtPrepare_MultiXact(void)
if (MultiXactIdIsValid(myOldestMember))
RegisterTwoPhaseRecord(TWOPHASE_RM_MULTIXACT_ID, 0,
- &myOldestMember, sizeof(MultiXactId));
+ &myOldestMember, MXID_SIZE);
}
/*
@@ -1832,7 +1835,7 @@ multixact_twophase_recover(TransactionId xid, uint16 info,
* Get the oldest member XID from the state file record, and set it in the
* OldestMemberMXactId slot reserved for this prepared transaction.
*/
- Assert(len == sizeof(MultiXactId));
+ Assert(len == MXID_SIZE);
oldestMember = *((MultiXactId *) recdata);
OldestMemberMXactId[dummyProcNumber] = oldestMember;
@@ -1848,7 +1851,7 @@ multixact_twophase_postcommit(TransactionId xid, uint16 info,
{
ProcNumber dummyProcNumber = TwoPhaseGetDummyProcNumber(xid, true);
- Assert(len == sizeof(MultiXactId));
+ Assert(len == MXID_SIZE);
OldestMemberMXactId[dummyProcNumber] = InvalidMultiXactId;
}
@@ -1877,7 +1880,7 @@ MultiXactShmemSize(void)
/* We need 2*MaxOldestSlot perBackendXactIds[] entries */
#define SHARED_MULTIXACT_STATE_SIZE \
add_size(offsetof(MultiXactStateData, perBackendXactIds), \
- mul_size(sizeof(MultiXactId) * 2, MaxOldestSlot))
+ mul_size(MXID_SIZE * 2, MaxOldestSlot))
size = SHARED_MULTIXACT_STATE_SIZE;
size = add_size(size, SimpleLruShmemSize(multixact_offset_buffers, 0));
@@ -2146,7 +2149,7 @@ TrimMultiXact(void)
offptr = (MultiXactOffset *) MultiXactOffsetCtl->shared->page_buffer[slotno];
offptr += entryno;
- MemSet(offptr, 0, BLCKSZ - (entryno * sizeof(MultiXactOffset)));
+ MemSet(offptr, 0, BLCKSZ - (entryno * MXOFF_SIZE));
MultiXactOffsetCtl->shared->page_dirty[slotno] = true;
LWLockRelease(lock);
diff --git a/src/bin/pg_resetwal/t/001_basic.pl b/src/bin/pg_resetwal/t/001_basic.pl
index 9829e48106..19fb6dc6a2 100644
--- a/src/bin/pg_resetwal/t/001_basic.pl
+++ b/src/bin/pg_resetwal/t/001_basic.pl
@@ -205,8 +205,20 @@ push @cmd,
'-c',
sprintf("%d,%d", hex($files[0]) == 0 ? 3 : hex($files[0]), hex($files[-1]));
+use constant SLRU_PAGES_PER_SEGMENT => 32;
+use constant MXOFF_SIZE => 4;
+use constant MXID_SIZE => 4;
+use constant MULTIXACT_MEMBERS_PER_MEMBERGROUP => 4;
+use constant MULTIXACT_FLAGBYTES_PER_GROUP => 4;
+use constant MULTIXACT_MEMBERGROUP_SIZE => MXID_SIZE * MULTIXACT_MEMBERS_PER_MEMBERGROUP + MULTIXACT_FLAGBYTES_PER_GROUP;
+use constant MULTIXACT_MEMBERGROUPS_PER_PAGE => int($blcksz / MULTIXACT_MEMBERGROUP_SIZE);
+use constant MULTIXACT_MEMBERS_PER_PAGE => MULTIXACT_MEMBERGROUPS_PER_PAGE * MULTIXACT_MEMBERS_PER_MEMBERGROUP;
+use constant MULTIXACT_OFFSETS_PER_PAGE => int($blcksz / MXOFF_SIZE);
+use constant CLOG_XACTS_PER_BYTE => 4;
+use constant CLOG_XACTS_PER_PAGE => $blcksz * CLOG_XACTS_PER_BYTE;
+
@files = get_slru_files('pg_multixact/offsets');
-$mult = 32 * $blcksz / 4;
+$mult = SLRU_PAGES_PER_SEGMENT * MULTIXACT_OFFSETS_PER_PAGE;
# -m argument is "new,old"
push @cmd, '-m',
sprintf("%d,%d",
@@ -214,11 +226,11 @@ push @cmd, '-m',
hex($files[0]) == 0 ? 1 : hex($files[0] * $mult));
@files = get_slru_files('pg_multixact/members');
-$mult = 32 * int($blcksz / 20) * 4;
+$mult = SLRU_PAGES_PER_SEGMENT * MULTIXACT_MEMBERS_PER_PAGE;
push @cmd, '-O', (hex($files[-1]) + 1) * $mult;
@files = get_slru_files('pg_xact');
-$mult = 32 * $blcksz * 4;
+$mult = SLRU_PAGES_PER_SEGMENT * CLOG_XACTS_PER_PAGE;
push @cmd,
'-u', (hex($files[0]) == 0 ? 3 : hex($files[0]) * $mult),
'-x', ((hex($files[-1]) + 1) * $mult);
--
2.34.1
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;