On 28/10/2026 17:59, Lionel Hamayon wrote:
Jasper's multixact.patch still applies cleanly to master, and with it:
- the repro above and the variants I tried pass (FOR UPDATE with a
key-changing update, ON CONFLICT DO UPDATE, MERGE, PL/pgSQL exception
blocks, a full page);
- make check and the isolation tests pass;
- on Luc's cost question: rebuilding a multixact with the same members
returns the existing one from the backend-local cache, so no extra
multixact IDs are used. I ran 500 TOAST updates while another
transaction held FOR KEY SHARE on the row, and both the patched and
unpatched builds used 2 multixacts. What remains is one
TransactionIdIsInProgress() call per member, and only on this path,
which as far as I can tell is reachable only from heap_update()'s
TOAST/new-page branch.
The patch still needs its comments updated: the function header and the
comment above the loop both still say the existing multixact is
returned as-is.
I looked at this patch and also the patch proposed at [1], and came up
with the attached version. The main difference is that it doesn't rely
on the cache to detect that the members haven't changed. It's probably
not significant performance-wise, but it's also very easy to track that
in MultiXactIdExpand() itself.
The extra TransactionIdIsInProgress() calls might matter if the "xid is
already a member" case was common. But I don't think it is. Notably,
heap_lock_tuple() doesn't rely on that but has different checks for
whether you're already holding the suitable lock. So if you do e.g.
"SELECT FOR KEY SHARE" twice on the same set of tuples, there's no
performance difference.
There's a missed opportunity in heap_update(): if the tuple is already
locked, there's no need to lock it again. If heap_update() checked for
that like heap_lock_tuple() does, then it it would never reach
MultiXactIdExpand() when the XID is already a member. We should perhaps
implement that in 'master'.
Anyway, thoughts on the attached patch?
Attached is a regression test for combocid.sql that goes on top of
Jasper's patch. It has three cases, and each one crashes an
assert-enabled build without the fix.
I kept only one of the three cases in the attached patch, since they all
reach the same codepath and assertion in the end. But thanks for
including them, it's always good to have more coverage during
development, even if not all of the test cases are added to the
permanent regression suite.
In full disclosure: I'm not a PostgreSQL internals developer. The
analysis and the test were done with an AI assistant (Claude). I checked
the results by running them, but I can't vouch for the reasoning in
depth, so please treat it accordingly. I hope that, together with
Jasper's patch, this is enough to get the fix in.
Thanks!
PS. While working on this, I notice another bug in the heap_update()
locking code, see
https://www.postgresql.org/message-id/[email protected].
It's unrelated to this one though, except that it's in the same piece of
code as the assertion.
[1]
https://www.postgresql.org/message-id/CAPgh2mJhTo2yq_vLrT6OZPn54hDxJpDVu%3D9L7Ay_M5qogNFuTw%40mail.gmail.com
- Heikki
From 2414ca1dada166769dbc7300dea9186a5a3214bf Mon Sep 17 00:00:00 2001
From: Heikki Linnakangas <[email protected]>
Date: Tue, 3 Nov 2026 16:08:07 +0200
Subject: [PATCH 1/1] Always prune dead members in MultiXactIdExpand, to fix
assertion
MultiXactIdExpand() has an early return that returns the original
multixid, when the transaction is already a member. The early return
happened before pruning members that are no longer of interest, e.g.
aborted transactions.
The comment in MultiXactIdExpand() says that removing the dead members
is not merely an optimization but needed for freezing, so it was
surprising that they were not always removed. I don't see a live bug
with freezing though.
However, it tripped this assertion in heap_update(). When
heap_update() needs to temporarily lock the tuple before updating it,
it calls compute_new_xmax_infomask(), which calls MultiXactIdExpand(),
and then asserts that:
Assert(HEAP_XMAX_IS_LOCKED_ONLY(infomask_lock_old_tuple));
The assumption makes sense at first glance: we've checked that the
tuple is not being updated, and we computed the infomask for locking
it, so surely the infomask should not indicate any updaters. However,
if the xmax was previously a multixid which contained an updating
transaction that is now aborted, and our transaction was already a
member, MultiXactIdExpand() returned the original multixid with the
aborted updating member included.
To fix, make MultiXactIdExpand() remove dead members, even if the
original multixid already contains the transaction being added. That
makes the assertion valid again.
Reported-by: Jasper Smit <[email protected]>
Reported-by: Lionel Hamayon <[email protected]>
Reported-by: RekGRpth <[email protected]>
Analyzed-by: Luc Vlaming Hummel <[email protected]>
Discussion: https://www.postgresql.org/message-id/CAOG+RQ6ECiiFH7bb7e1M0bkd0whprp=T0u=rvarsapogry-...@mail.gmail.com
Discussion: https://www.postgresql.org/message-id/CAPgh2mJhTo2yq_vLrT6OZPn54hDxJpDVu=9l7ay_m5qognf...@mail.gmail.com
Backpatch-through: 14
---
src/backend/access/transam/multixact.c | 65 ++++++++++++++++----------
src/test/regress/expected/combocid.out | 49 +++++++++++++++++++
src/test/regress/sql/combocid.sql | 44 +++++++++++++++++
3 files changed, 133 insertions(+), 25 deletions(-)
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 4d5fd9f8eb2..4f8829d5e59 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -416,6 +416,7 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
int nmembers;
int i;
int j;
+ bool alreadyMember;
Assert(MultiXactIdIsValid(multi));
Assert(TransactionIdIsValid(xid));
@@ -453,22 +454,6 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
return newMulti;
}
- /*
- * If the TransactionId is already a member of the MultiXactId with the
- * same status, just return the existing MultiXactId.
- */
- for (i = 0; i < nmembers; i++)
- {
- if (TransactionIdEquals(members[i].xid, xid) &&
- (members[i].status == status))
- {
- debug_elog4(DEBUG2, "Expand: %u is already a member of %u",
- xid, multi);
- pfree(members);
- return multi;
- }
- }
-
/*
* Determine which of the members of the MultiXactId are still of
* interest. This is any running transaction, and also any transaction
@@ -477,33 +462,63 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status)
* update Xid in a multixact would cause errors elsewhere.)
*
* Removing dead members is not just an optimization: freezing of tuples
- * whose Xmax are multis depends on this behavior.
+ * whose Xmax are multis depends on this behavior. This can also help the
+ * caller to calculate a weaker overall lock level for this multixid. If
+ * e.g. the caller has already determined that the updating transaction is
+ * not running, it would be surprising for the result to still contain a
+ * member with MultiXactStatusUpdate.
*
* Note we have the same race condition here as above: j could be 0 at the
* end of the loop.
*/
newMembers = palloc_array(MultiXactMember, nmembers + 1);
+ alreadyMember = false;
for (i = 0, j = 0; i < nmembers; i++)
{
- if (TransactionIdIsInProgress(members[i].xid) ||
- (ISUPDATE_from_mxstatus(members[i].status) &&
- TransactionIdDidCommit(members[i].xid)))
+ if (TransactionIdEquals(members[i].xid, xid) &&
+ (members[i].status == status))
+ {
+ alreadyMember = true;
+ debug_elog4(DEBUG2, "Expand: %u is already a member of %u",
+ xid, multi);
+ newMembers[j].xid = xid;
+ newMembers[j++].status = status;
+ }
+ else if (TransactionIdIsInProgress(members[i].xid) ||
+ (ISUPDATE_from_mxstatus(members[i].status) &&
+ TransactionIdDidCommit(members[i].xid)))
{
newMembers[j].xid = members[i].xid;
newMembers[j++].status = members[i].status;
}
}
- newMembers[j].xid = xid;
- newMembers[j++].status = status;
- newMulti = MultiXactIdCreateFromMembers(j, newMembers);
+ /*
+ * If the TransactionId is already a member of the MultiXactId with the
+ * same status, and there were no dead members to remove, just return the
+ * existing MultiXactId. (The caching in MultiXactIdCreateFromMembers()
+ * would detect this case too, but let's save the effort.)
+ */
+ if (alreadyMember && i == j)
+ {
+ newMulti = multi;
+ debug_elog3(DEBUG2, "Expand: returning original multi %u", multi);
+ }
+ else
+ {
+ if (!alreadyMember)
+ {
+ newMembers[j].xid = xid;
+ newMembers[j++].status = status;
+ }
+ newMulti = MultiXactIdCreateFromMembers(j, newMembers);
+ debug_elog3(DEBUG2, "Expand: returning new multi %u", newMulti);
+ }
pfree(members);
pfree(newMembers);
- debug_elog3(DEBUG2, "Expand: returning new multi %u", newMulti);
-
return newMulti;
}
diff --git a/src/test/regress/expected/combocid.out b/src/test/regress/expected/combocid.out
index 2bf080b2ae2..daa1fcfdffe 100644
--- a/src/test/regress/expected/combocid.out
+++ b/src/test/regress/expected/combocid.out
@@ -167,3 +167,52 @@ SELECT * FROM testcase WHERE id = 1 FOR UPDATE;
ROLLBACK;
DROP TABLE testcase;
+-- Test for bug reported in
+-- CAOG+RQ6ECiiFH7bb7e1M0bkd0whprp=T0u=rvarsapogry-...@mail.gmail.com.
+-- The bug was not in combocid handling, but the sequence of commands
+-- is very similar to the previous case.
+--
+-- If the new tuple of an UPDATE needs toasting or doesn't fit on the
+-- page, heap_update() temporarily releases the buffer lock. Before
+-- doing so, it marks the old tuple as locked, and that tripped an
+-- assertion if the tuple was already locked in the same mode earlier
+-- in the transaction, and the multixid also contained an aborted
+-- updating transaction. The assertion assumed that the multixid on
+-- the tuple was lock-only, but didn't take the possibility of an
+-- aborted updating XID into account. That was fixed by making
+-- MultiXactIdExpand() to remove the aborted XID in all cases, making
+-- the assertion valid again.
+CREATE TABLE testcase(
+ id int PRIMARY KEY,
+ val text
+);
+INSERT INTO testcase VALUES (1, '');
+BEGIN;
+-- Lock the row in the top transaction
+SELECT id FROM testcase WHERE id = 1 FOR NO KEY UPDATE;
+ id
+----
+ 1
+(1 row)
+
+-- Update the row in a subtransaction, creating a multixid, and abort
+-- the subtransaction.
+SAVEPOINT subxact;
+UPDATE testcase SET val = 'subxact' WHERE id = 1;
+ROLLBACK TO SAVEPOINT subxact;
+RELEASE SAVEPOINT subxact;
+-- Update the row again in the same top-level transaction. The new
+-- value is large so that it needs to be toasted; that triggers the
+-- codepath in heap_update() that first locks the tuple. Thanks to
+-- the FOR NO KEY UPDATE select earlier in the transaction, the tuple
+-- is already locked.
+UPDATE testcase SET val = repeat('x', 3000) WHERE id = 1;
+-- Should return the updated tuple
+SELECT id, length(val) FROM testcase;
+ id | length
+----+--------
+ 1 | 3000
+(1 row)
+
+COMMIT;
+DROP TABLE testcase;
diff --git a/src/test/regress/sql/combocid.sql b/src/test/regress/sql/combocid.sql
index a5cdf6dba29..c3406427f52 100644
--- a/src/test/regress/sql/combocid.sql
+++ b/src/test/regress/sql/combocid.sql
@@ -109,3 +109,47 @@ ROLLBACK TO SAVEPOINT subxact;
SELECT * FROM testcase WHERE id = 1 FOR UPDATE;
ROLLBACK;
DROP TABLE testcase;
+
+-- Test for bug reported in
+-- CAOG+RQ6ECiiFH7bb7e1M0bkd0whprp=T0u=rvarsapogry-...@mail.gmail.com.
+-- The bug was not in combocid handling, but the sequence of commands
+-- is very similar to the previous case.
+--
+-- If the new tuple of an UPDATE needs toasting or doesn't fit on the
+-- page, heap_update() temporarily releases the buffer lock. Before
+-- doing so, it marks the old tuple as locked, and that tripped an
+-- assertion if the tuple was already locked in the same mode earlier
+-- in the transaction, and the multixid also contained an aborted
+-- updating transaction. The assertion assumed that the multixid on
+-- the tuple was lock-only, but didn't take the possibility of an
+-- aborted updating XID into account. That was fixed by making
+-- MultiXactIdExpand() to remove the aborted XID in all cases, making
+-- the assertion valid again.
+CREATE TABLE testcase(
+ id int PRIMARY KEY,
+ val text
+);
+INSERT INTO testcase VALUES (1, '');
+BEGIN;
+-- Lock the row in the top transaction
+SELECT id FROM testcase WHERE id = 1 FOR NO KEY UPDATE;
+
+-- Update the row in a subtransaction, creating a multixid, and abort
+-- the subtransaction.
+SAVEPOINT subxact;
+UPDATE testcase SET val = 'subxact' WHERE id = 1;
+ROLLBACK TO SAVEPOINT subxact;
+RELEASE SAVEPOINT subxact;
+
+-- Update the row again in the same top-level transaction. The new
+-- value is large so that it needs to be toasted; that triggers the
+-- codepath in heap_update() that first locks the tuple. Thanks to
+-- the FOR NO KEY UPDATE select earlier in the transaction, the tuple
+-- is already locked.
+UPDATE testcase SET val = repeat('x', 3000) WHERE id = 1;
+
+-- Should return the updated tuple
+SELECT id, length(val) FROM testcase;
+COMMIT;
+
+DROP TABLE testcase;
--
2.47.3
