From 393f28853b920b84df3629a6c61297e8f457f2d6 Mon Sep 17 00:00:00 2001 From: Andrey Rachitskiy Date: Mon, 2 Nov 2026 13:00:37 +0500 Subject: [PATCH] Fix MultiXactIdExpand reusing a multixact with dead members MultiXactIdExpand returned its input when the caller was already a member with the same status. That return ran before dead members were removed, so an aborted updater could stay in the result. heap_update used this MultiXactId for a temporary lock, the infomask was not HEAP_XMAX_LOCK_ONLY, and an assertion build failed. Return the existing MultiXactId only when every member is still of interest. Otherwise create a new one from the members that remain. Do not add the caller a second time. Author: Georgy Shelkovy Reviewed-by: Andrey Rachitskiy --- src/backend/access/transam/multixact.c | 69 +++++++++++++------ .../expected/multixact-expand-savepoint.out | 28 ++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/multixact-expand-savepoint.spec | 43 ++++++++++++ 4 files changed, 121 insertions(+), 20 deletions(-) create mode 100644 src/test/isolation/expected/multixact-expand-savepoint.out create mode 100644 src/test/isolation/specs/multixact-expand-savepoint.spec diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c index 4d5fd9f8eb2..4b8f3c46fc9 100644 --- a/src/backend/access/transam/multixact.c +++ b/src/backend/access/transam/multixact.c @@ -388,12 +388,28 @@ MultiXactIdCreate(TransactionId xid1, MultiXactStatus status1, return newMulti; } +/* + * MultiXactMemberStillOfInterest + * Any running transaction is still of interest. So is a transaction + * that grabbed something stronger than just a lock and committed. + */ +static bool +MultiXactMemberStillOfInterest(MultiXactMember *member) +{ + return TransactionIdIsInProgress(member->xid) || + (ISUPDATE_from_mxstatus(member->status) && + TransactionIdDidCommit(member->xid)); +} + /* * MultiXactIdExpand * Add a TransactionId to a pre-existing MultiXactId. * * If the TransactionId is already a member of the passed MultiXactId with the - * same status, just return it as-is. + * same status, and every member is still of interest, return that + * MultiXactId as-is. If some member is no longer of interest, create a new + * MultiXactId from the members that remain. Do not add the TransactionId + * again when it is still among them. * * Note that we do NOT actually modify the membership of a pre-existing * MultiXactId; instead we create a new one. This is necessary to avoid @@ -453,22 +469,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 @@ -486,15 +486,44 @@ MultiXactIdExpand(MultiXactId multi, TransactionId xid, MultiXactStatus status) for (i = 0, j = 0; i < nmembers; i++) { - if (TransactionIdIsInProgress(members[i].xid) || - (ISUPDATE_from_mxstatus(members[i].status) && - TransactionIdDidCommit(members[i].xid))) + if (MultiXactMemberStillOfInterest(&members[i])) { newMembers[j].xid = members[i].xid; newMembers[j++].status = members[i].status; } } + /* + * If the TransactionId is already a member with the same status, do not + * add it again. The loop above only copies or skips members, so equal + * counts mean nothing was dropped and the existing MultiXactId is still + * the right set. Otherwise build a new one from the members that remain. + */ + for (i = 0; i < j; i++) + { + if (TransactionIdEquals(newMembers[i].xid, xid) && + (newMembers[i].status == status)) + { + if (j == nmembers) + { + debug_elog4(DEBUG2, "Expand: %u is already a member of %u", + xid, multi); + pfree(members); + pfree(newMembers); + return multi; + } + + newMulti = MultiXactIdCreateFromMembers(j, newMembers); + + pfree(members); + pfree(newMembers); + + debug_elog3(DEBUG2, "Expand: returning new multi %u", newMulti); + + return newMulti; + } + } + newMembers[j].xid = xid; newMembers[j++].status = status; newMulti = MultiXactIdCreateFromMembers(j, newMembers); diff --git a/src/test/isolation/expected/multixact-expand-savepoint.out b/src/test/isolation/expected/multixact-expand-savepoint.out new file mode 100644 index 00000000000..1abd069e06f --- /dev/null +++ b/src/test/isolation/expected/multixact-expand-savepoint.out @@ -0,0 +1,28 @@ +Parsed test spec with 2 sessions + +starting permutation: s1b s1l s1sv s1u s1rbs s1rel s1u2 s1len s1c s2sel +step s1b: BEGIN; +step s1l: SELECT id FROM multixact_expand WHERE id = 1 FOR NO KEY UPDATE; +id +-- + 1 +(1 row) + +step s1sv: SAVEPOINT s; +step s1u: UPDATE multixact_expand SET val = 'sub' WHERE id = 1; +step s1rbs: ROLLBACK TO SAVEPOINT s; +step s1rel: RELEASE SAVEPOINT s; +step s1u2: UPDATE multixact_expand SET val = repeat('md5', 4000) WHERE id = 1; +step s1len: SELECT length(val) FROM multixact_expand WHERE id = 1; +length +------ + 12000 +(1 row) + +step s1c: COMMIT; +step s2sel: SELECT id FROM multixact_expand; +id +-- + 1 +(1 row) + diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index bf9a2037c70..9c1b69584a1 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -71,6 +71,7 @@ test: aborted-keyrevoke test: multixact-no-deadlock test: multixact-no-forget test: multixact-stats +test: multixact-expand-savepoint test: lock-committed-update test: lock-committed-keyupdate test: update-locked-tuple diff --git a/src/test/isolation/specs/multixact-expand-savepoint.spec b/src/test/isolation/specs/multixact-expand-savepoint.spec new file mode 100644 index 00000000000..547969a2914 --- /dev/null +++ b/src/test/isolation/specs/multixact-expand-savepoint.spec @@ -0,0 +1,43 @@ +# MultiXactIdExpand must not reuse a multixact that still contains a member +# which is no longer of interest. +# +# s1 locks the row FOR NO KEY UPDATE, updates it in a subtransaction, rolls +# that subtransaction back, and releases the savepoint. The next statement +# therefore runs as s1 itself. Its xid is already a member of the row's +# xmax, with status ForNoKeyUpdate. The second update changes val, which is +# not a key column, so the temporary lock in heap_update uses that same +# status. The new value is toasted, so heap_update takes that temporary +# lock. The xmax computed there has to be lock-only. An aborted updater +# left in the multixact used to leave HEAP_XMAX_LOCK_ONLY unset, and an +# assertion build failed in Assert(HEAP_XMAX_IS_LOCKED_ONLY). +# +# Updating id instead of val would take ForUpdate for the temporary lock. +# That status does not match, and the old code already pruned. + +setup +{ + DROP TABLE IF EXISTS multixact_expand; + CREATE TABLE multixact_expand (id int primary key, val text); + INSERT INTO multixact_expand VALUES (1, 'x'); +} + +teardown +{ + DROP TABLE multixact_expand; +} + +session s1 +step s1b { BEGIN; } +step s1l { SELECT id FROM multixact_expand WHERE id = 1 FOR NO KEY UPDATE; } +step s1sv { SAVEPOINT s; } +step s1u { UPDATE multixact_expand SET val = 'sub' WHERE id = 1; } +step s1rbs { ROLLBACK TO SAVEPOINT s; } +step s1rel { RELEASE SAVEPOINT s; } +step s1u2 { UPDATE multixact_expand SET val = repeat('md5', 4000) WHERE id = 1; } +step s1len { SELECT length(val) FROM multixact_expand WHERE id = 1; } +step s1c { COMMIT; } + +session s2 +step s2sel { SELECT id FROM multixact_expand; } + +permutation s1b s1l s1sv s1u s1rbs s1rel s1u2 s1len s1c s2sel -- 2.53.0