Bugfix and improvements in multixact.c
Hi!
While working on a making multix xact offsets 64-bit [0]/messages/by-id/ff143b24-a093-40da-9833-d36b83726bdf@iki.fi I've discovered a
minor issue. The
thing is that type 'xid' is used in all macro, but it doesn't correct.
Appropriate MultiXactId or
MultiXactOffset should be used, actually.
And the second thing, as Heikki Linnakangas points out, args naming is also
misleading.
Since, these problems are not the point of thread [0]/messages/by-id/ff143b24-a093-40da-9833-d36b83726bdf@iki.fi, I decided to create
this discussion.
And here is the patch set addressing mentioned issues (0001 and 0002).
Additionally, I made an optional patch 0003 to switch from macro to inline
functions. For
me, personally, use of macro functions is justified if we are dealing with
different argument
types, to make polymorphic call. Which is not the case here. So, we can
have more
control over types and still generate the same code in terms of speed.
See https://godbolt.org/z/KM8voadhs Starting from O1 function is inlined,
thus no
overhead is noticeable. Anyway, it's up to the actual commiter to decide
does it worth it
or not. Again, this particular patch 0003 is completely optional.
As always, any opinions and reviews are very welcome!
[0]: /messages/by-id/ff143b24-a093-40da-9833-d36b83726bdf@iki.fi
/messages/by-id/ff143b24-a093-40da-9833-d36b83726bdf@iki.fi
--
Best regards,
Maxim Orlov.
Attachments:
v1-0003-Switch-from-macro-functions-to-inline-in-multixac.patchapplication/octet-stream; name=v1-0003-Switch-from-macro-functions-to-inline-in-multixac.patchDownload
From 415344731b62c2095f165acc5f0c4f7750458b21 Mon Sep 17 00:00:00 2001
From: Maxim Orlov <m.orlov@postgrespro.ru>
Date: Fri, 14 Jun 2024 15:41:12 +0300
Subject: [PATCH v1 3/3] Switch from macro functions to inline in multixact.
Author: Maxim Orlov <orlovmg@gmail.com>
Reviewed-by: ...
Discussions:
https://www.postgresql.org/message-id/ff143b24-a093-40da-9833-d36b83726bdf%40iki.fi
---
src/backend/access/transam/multixact.c | 84 ++++++++++++++++++++------
1 file changed, 65 insertions(+), 19 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 53a81b3ef9..bcbee48270 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -108,11 +108,23 @@
/* We need four bytes per offset */
#define MULTIXACT_OFFSETS_PER_PAGE (BLCKSZ / sizeof(MultiXactOffset))
-#define MultiXactIdToOffsetPage(multi) \
- ((multi) / (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
-#define MultiXactIdToOffsetEntry(multi) \
- ((multi) % (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
-#define MultiXactIdToOffsetSegment(multi) (MultiXactIdToOffsetPage(multi) / SLRU_PAGES_PER_SEGMENT)
+static inline int64
+MultiXactIdToOffsetPage(MultiXactId multi)
+{
+ return multi / MULTIXACT_OFFSETS_PER_PAGE;
+}
+
+static inline int
+MultiXactIdToOffsetEntry(MultiXactId multi)
+{
+ return multi % MULTIXACT_OFFSETS_PER_PAGE;
+}
+
+static inline int
+MultiXactIdToOffsetSegment(MultiXactId multi)
+{
+ return MultiXactIdToOffsetPage(multi) / SLRU_PAGES_PER_SEGMENT;
+}
/*
* The situation for members is a bit more complex: we store one byte of
@@ -156,30 +168,64 @@
((uint32) ((0xFFFFFFFF % MULTIXACT_MEMBERS_PER_PAGE) + 1))
/* page in which a member is to be found */
-#define MXOffsetToMemberPage(offset) ((offset) / (MultiXactOffset) MULTIXACT_MEMBERS_PER_PAGE)
-#define MXOffsetToMemberSegment(offset) (MXOffsetToMemberPage(offset) / SLRU_PAGES_PER_SEGMENT)
+static inline int64
+MXOffsetToMemberPage(MultiXactOffset offset)
+{
+ return offset / MULTIXACT_MEMBERS_PER_PAGE;
+}
+
+static inline int
+MXOffsetToMemberSegment(MultiXactOffset offset)
+{
+ return MXOffsetToMemberPage(offset) / SLRU_PAGES_PER_SEGMENT;
+}
/* Location (byte offset within page) of flag word for a given member */
-#define MXOffsetToFlagsOffset(offset) \
- ((((offset) / (MultiXactOffset) MULTIXACT_MEMBERS_PER_MEMBERGROUP) % \
- (MultiXactOffset) MULTIXACT_MEMBERGROUPS_PER_PAGE) * \
- (MultiXactOffset) MULTIXACT_MEMBERGROUP_SIZE)
-#define MXOffsetToFlagsBitShift(offset) \
- (((offset) % (MultiXactOffset) MULTIXACT_MEMBERS_PER_MEMBERGROUP) * \
- MXACT_MEMBER_BITS_PER_XACT)
+static inline int
+MXOffsetToFlagsOffset(MultiXactOffset offset)
+{
+ int flagsoff;
+
+ offset /= MULTIXACT_MEMBERS_PER_MEMBERGROUP;
+ offset %= MULTIXACT_MEMBERGROUPS_PER_PAGE;
+ flagsoff = offset * MULTIXACT_MEMBERGROUP_SIZE;
+
+ return flagsoff;
+}
+
+static inline int
+MXOffsetToFlagsBitShift(MultiXactOffset offset)
+{
+ int bshift;
+
+ offset %= MULTIXACT_MEMBERS_PER_MEMBERGROUP;
+ bshift = offset * MXACT_MEMBER_BITS_PER_XACT;
+
+ return bshift;
+}
/* Location (byte offset within page) of TransactionId of given member */
-#define MXOffsetToMemberOffset(offset) \
- (MXOffsetToFlagsOffset(offset) + MULTIXACT_FLAGBYTES_PER_GROUP + \
- ((offset) % MULTIXACT_MEMBERS_PER_MEMBERGROUP) * sizeof(TransactionId))
+static inline int
+MXOffsetToMemberOffset(MultiXactOffset offset)
+{
+ int memberoff;
+
+ memberoff = MXOffsetToFlagsOffset(offset) + MULTIXACT_FLAGBYTES_PER_GROUP +
+ (offset % MULTIXACT_MEMBERS_PER_MEMBERGROUP) * sizeof(TransactionId);
+
+ return memberoff;
+}
/* Multixact members wraparound thresholds. */
#define MULTIXACT_MEMBER_SAFE_THRESHOLD (MaxMultiXactOffset / 2)
#define MULTIXACT_MEMBER_DANGER_THRESHOLD \
(MaxMultiXactOffset - MaxMultiXactOffset / 4)
-#define PreviousMultiXactId(multi) \
- ((multi) == FirstMultiXactId ? MaxMultiXactId : (multi) - 1)
+static inline MultiXactId
+PreviousMultiXactId(MultiXactId multi)
+{
+ return multi == FirstMultiXactId ? MaxMultiXactId : multi - 1;
+}
/*
* Links to shared-memory data structures for MultiXact control
--
2.44.0
v1-0002-Fix-using-of-MultiXactOffset-type-insted-of-Trans.patchapplication/octet-stream; name=v1-0002-Fix-using-of-MultiXactOffset-type-insted-of-Trans.patchDownload
From bd624d1a5f9f504fcdd57dcd9db1da6a80915efa Mon Sep 17 00:00:00 2001
From: Maxim Orlov <m.orlov@postgrespro.ru>
Date: Fri, 14 Jun 2024 15:29:14 +0300
Subject: [PATCH v1 2/3] Fix using of MultiXactOffset type insted of
TransactionId in multixact.
Using of TransactionId type in casting was erroneous. It wasn't led to any real
problem, since TransactionId and MultiXactOffset were both typedefs of uint32.
But it is semantically wrong and should be fixed anyway.
Author: Maxim Orlov <orlovmg@gmail.com>
Reviewed-by: ...
Discussions:
https://www.postgresql.org/message-id/ff143b24-a093-40da-9833-d36b83726bdf%40iki.fi
---
src/backend/access/transam/multixact.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 3c76cc797c..53a81b3ef9 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -156,16 +156,16 @@
((uint32) ((0xFFFFFFFF % MULTIXACT_MEMBERS_PER_PAGE) + 1))
/* page in which a member is to be found */
-#define MXOffsetToMemberPage(offset) ((offset) / (TransactionId) MULTIXACT_MEMBERS_PER_PAGE)
+#define MXOffsetToMemberPage(offset) ((offset) / (MultiXactOffset) MULTIXACT_MEMBERS_PER_PAGE)
#define MXOffsetToMemberSegment(offset) (MXOffsetToMemberPage(offset) / SLRU_PAGES_PER_SEGMENT)
/* Location (byte offset within page) of flag word for a given member */
#define MXOffsetToFlagsOffset(offset) \
- ((((offset) / (TransactionId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) % \
- (TransactionId) MULTIXACT_MEMBERGROUPS_PER_PAGE) * \
- (TransactionId) MULTIXACT_MEMBERGROUP_SIZE)
+ ((((offset) / (MultiXactOffset) MULTIXACT_MEMBERS_PER_MEMBERGROUP) % \
+ (MultiXactOffset) MULTIXACT_MEMBERGROUPS_PER_PAGE) * \
+ (MultiXactOffset) MULTIXACT_MEMBERGROUP_SIZE)
#define MXOffsetToFlagsBitShift(offset) \
- (((offset) % (TransactionId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) * \
+ (((offset) % (MultiXactOffset) MULTIXACT_MEMBERS_PER_MEMBERGROUP) * \
MXACT_MEMBER_BITS_PER_XACT)
/* Location (byte offset within page) of TransactionId of given member */
--
2.44.0
v1-0001-Get-rid-of-misleading-xid-arg-name-in-multixact.patchapplication/octet-stream; name=v1-0001-Get-rid-of-misleading-xid-arg-name-in-multixact.patchDownload
From 21ff17540d217b601426bf6f71aba5ea74d0d1f0 Mon Sep 17 00:00:00 2001
From: Maxim Orlov <m.orlov@postgrespro.ru>
Date: Fri, 14 Jun 2024 15:09:44 +0300
Subject: [PATCH v1 1/3] Get rid of misleading xid arg name in multixact.
Using 'xid' argument name in macro functions was misleading. Switch to the
proper 'multi' and 'offset' names.
Author: Maxim Orlov <orlovmg@gmail.com>
Reviewed-by: ...
Discussions:
https://www.postgresql.org/message-id/ff143b24-a093-40da-9833-d36b83726bdf%40iki.fi
---
src/backend/access/transam/multixact.c | 32 +++++++++++++-------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 54c916e034..3c76cc797c 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -108,11 +108,11 @@
/* We need four bytes per offset */
#define MULTIXACT_OFFSETS_PER_PAGE (BLCKSZ / sizeof(MultiXactOffset))
-#define MultiXactIdToOffsetPage(xid) \
- ((xid) / (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
-#define MultiXactIdToOffsetEntry(xid) \
- ((xid) % (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
-#define MultiXactIdToOffsetSegment(xid) (MultiXactIdToOffsetPage(xid) / SLRU_PAGES_PER_SEGMENT)
+#define MultiXactIdToOffsetPage(multi) \
+ ((multi) / (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
+#define MultiXactIdToOffsetEntry(multi) \
+ ((multi) % (MultiXactOffset) MULTIXACT_OFFSETS_PER_PAGE)
+#define MultiXactIdToOffsetSegment(multi) (MultiXactIdToOffsetPage(multi) / SLRU_PAGES_PER_SEGMENT)
/*
* The situation for members is a bit more complex: we store one byte of
@@ -156,30 +156,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(offset) ((offset) / (TransactionId) MULTIXACT_MEMBERS_PER_PAGE)
+#define MXOffsetToMemberSegment(offset) (MXOffsetToMemberPage(offset) / 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) % \
+#define MXOffsetToFlagsOffset(offset) \
+ ((((offset) / (TransactionId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) % \
(TransactionId) MULTIXACT_MEMBERGROUPS_PER_PAGE) * \
(TransactionId) MULTIXACT_MEMBERGROUP_SIZE)
-#define MXOffsetToFlagsBitShift(xid) \
- (((xid) % (TransactionId) MULTIXACT_MEMBERS_PER_MEMBERGROUP) * \
+#define MXOffsetToFlagsBitShift(offset) \
+ (((offset) % (TransactionId) 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(offset) \
+ (MXOffsetToFlagsOffset(offset) + MULTIXACT_FLAGBYTES_PER_GROUP + \
+ ((offset) % 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(multi) \
+ ((multi) == FirstMultiXactId ? MaxMultiXactId : (multi) - 1)
/*
* Links to shared-memory data structures for MultiXact control
--
2.44.0
On 14/06/2024 16:56, Maxim Orlov wrote:
Hi!
While working on a making multix xact offsets 64-bit [0] I've
discovered a minor issue. The thing is that type 'xid' is used in
all macro, but it doesn't correct. Appropriate MultiXactId or
MultiXactOffset should be used, actually.And the second thing, as Heikki Linnakangas points out, args naming
is also misleading.
Thanks!
Looks good to me at a quick glance. I'll try to review and commit these
properly by Monday.
Additionally, I made an optional patch 0003 to switch from macro to
inline functions. For me, personally, use of macro functions is
justified if we are dealing with different argument types, to make
polymorphic call. Which is not the case here. So, we can have more
control over types and still generate the same code in terms of
speed. See https://godbolt.org/z/KM8voadhs Starting from O1 function
is inlined, thus no overhead is noticeable. Anyway, it's up to the
actual commiter to decide does it worth it or not. Again, this
particular patch 0003 is completely optional.
I agree static inline functions are generally easier to work with than
macros. These particular macros were not too bad, though.
I'll bite the bullet and commit this one too unless someone objects.
It's late in the v17 release cycle, but these are local to multixact.c
so there's no risk of breaking extensions, and it seems good to do it
now since we're modifying the macros anyway.
--
Heikki Linnakangas
Neon (https://neon.tech)
On 14/06/2024 16:56, Maxim Orlov wrote:
+static inline int +MXOffsetToFlagsOffset(MultiXactOffset offset) +{ + int flagsoff; + + offset /= MULTIXACT_MEMBERS_PER_MEMBERGROUP; + offset %= MULTIXACT_MEMBERGROUPS_PER_PAGE; + flagsoff = offset * MULTIXACT_MEMBERGROUP_SIZE; + + return flagsoff; +}
I found this reuse of the 'offset' variable a bit confusing, so I added
separate local variables for each step.
Committed with that change, thanks!
--
Heikki Linnakangas
Neon (https://neon.tech)