From 2414ca1dada166769dbc7300dea9186a5a3214bf Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas 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 Reported-by: Lionel Hamayon Reported-by: RekGRpth Analyzed-by: Luc Vlaming Hummel Discussion: https://www.postgresql.org/message-id/CAOG+RQ6ECiiFH7bb7e1M0bkd0whprp=T0u=rvARSaPOGrY-NDQ@mail.gmail.com Discussion: https://www.postgresql.org/message-id/CAPgh2mJhTo2yq_vLrT6OZPn54hDxJpDVu=9L7Ay_M5qogNFuTw@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-NDQ@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-NDQ@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