From 926fa68c8bc8ec6c58c7356476af1eee0b624e0b Mon Sep 17 00:00:00 2001 From: "chee.wooson" Date: Thu, 3 Sep 2026 17:08:56 +0800 Subject: [PATCH v2] Avoid carrying self lock-only xmax to updated tuple heap_update() normally carries pre-existing tuple lockers from the old tuple version to the new one. That behavior is required when the old tuple is locked by another transaction, in particular for FOR KEY SHARE locks taken by referential-integrity checks. However, when the old tuple's xmax is a single-XID, lock-only xmax, and that XID is the same XID that is now updating the tuple, there is no independent locker to carry forward. In the single-XID case, heap_update() carries the old xmax to the new tuple only as HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY, with no HEAP_KEYS_UPDATED bit. That is useful for preserving a remote key-share locker, but it is redundant for a self-owned lock. The new tuple's xmin is the updating XID, so other transactions cannot treat the new tuple as a visible committed tuple while the update is in progress, and the self-owned lock ends when that same transaction ends. Treat such a same-XID lock-only xmax as having no remaining locker to carry to the new tuple. Remote lockers, different subtransactions, and MultiXacts keep the existing behavior. Update the existing ON CONFLICT DO UPDATE regression test that explicitly tracked this behavior. Discussion: https://postgr.es/m/20190724232439.lpxzjw2jg3ukgcqn@alap3.anarazel.de --- src/backend/access/heap/README.tuplock | 38 ++++++++++++++++++++++++++ src/backend/access/heap/heapam.c | 13 +++++++++ src/test/regress/expected/update.out | 6 ++-- src/test/regress/sql/update.sql | 6 ++-- 4 files changed, 57 insertions(+), 6 deletions(-) diff --git a/src/backend/access/heap/README.tuplock b/src/backend/access/heap/README.tuplock index 16f7d78b7d2..6d14d9649b5 100644 --- a/src/backend/access/heap/README.tuplock +++ b/src/backend/access/heap/README.tuplock @@ -73,6 +73,44 @@ in the tuple itself. We do this by storing the locker's Xid in XMAX, and setting infomask bits specifying the locking strength. See "Infomask Bits" below for details on the bit patterns we use. +Carrying locks across UPDATE +---------------------------- + +When UPDATE creates a new tuple version, it normally carries pre-existing +lockers from the old tuple to the new tuple. That is necessary when another +transaction has locked the old tuple, since the lock must continue to protect +the updated version. This is essential for referential-integrity checks that +use FOR KEY SHARE locks. + +There is a narrow exception when the old tuple's XMAX is a single XID that is +LOCK_ONLY, and that XID is the same XID that is now updating the tuple. In +that case, there is no independent locker to carry forward. + +For a single-XID XMAX carried to the new tuple, heap_update() does not preserve +the old tuple lock's original strength. The carried state is encoded as +HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY, with no HEAP_KEYS_UPDATED bit. +That representation is enough for the case that needs carrying: a different +transaction's FOR KEY SHARE lock, such as one taken by a referential-integrity +check. It is not a mechanism for preserving a self-held FOR UPDATE or FOR NO +KEY UPDATE lock on the new tuple version. + +The same-XID case is redundant for a simpler reason. The new tuple's XMIN is +the updating XID, so other transactions cannot treat the new version as a +visible committed tuple while the update is in progress. Once the updating +transaction ends, any self-owned tuple lock ends with it. A self-owned +KEYSHR_LOCK | LOCK_ONLY marker carried to the new tuple therefore has no +remaining concurrency or referential-integrity role. + +The old tuple still records the update in its XMAX. If the old locker and the +updater are the same XID, compute_new_xmax_infomask() can fold those states +while preparing the old tuple's XMAX, but the safety of dropping the carried +new-tuple XMAX does not depend on comparing the old lock strength with the +UPDATE lock strength. + +This exception does not apply when the old locker is another transaction, a +different subtransaction of the current transaction, or a MultiXact. Those +cases still need to preserve the lock information on the new tuple. + MultiXacts ---------- diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 72d6541734c..135569eaa63 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3628,6 +3628,19 @@ l2: TransactionIdDidAbort(update_xact)) can_continue = true; } + else if (HEAP_XMAX_IS_LOCKED_ONLY(infomask) && + TransactionIdEquals(xwait, xid)) + { + /* + * At this point Xmax is known not to be a MultiXactId. The tuple + * is locked only by the same XID that is now updating it, so + * there is no independent locker to carry forward to the new + * tuple. The update itself is recorded in the old tuple's Xmax. + */ + checked_lockers = true; + locker_remains = false; + can_continue = true; + } else if (TransactionIdIsCurrentTransactionId(xwait)) { /* diff --git a/src/test/regress/expected/update.out b/src/test/regress/expected/update.out index eef2bac1cbf..fd6d5b5508f 100644 --- a/src/test/regress/expected/update.out +++ b/src/test/regress/expected/update.out @@ -246,11 +246,11 @@ INSERT INTO upsert_test VALUES (2, 'Beeble') ON CONFLICT(a) upsert_test | t | t (1 row) --- currently xmax is set after a conflict - that's probably not good, --- but it seems worthwhile to have to be explicit if that changes. +-- xmax is not carried forward after a conflict if the only prior locker +-- is the same transaction that is now updating the tuple. INSERT INTO upsert_test VALUES (2, 'Brox') ON CONFLICT(a) DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a) - RETURNING tableoid::regclass, xmin = pg_current_xact_id()::xid AS xmin_correct, xmax = pg_current_xact_id()::xid AS xmax_correct; + RETURNING tableoid::regclass, xmin = pg_current_xact_id()::xid AS xmin_correct, xmax = 0 AS xmax_correct; tableoid | xmin_correct | xmax_correct -------------+--------------+-------------- upsert_test | t | t diff --git a/src/test/regress/sql/update.sql b/src/test/regress/sql/update.sql index 8b4707eb9c3..3c2695d9567 100644 --- a/src/test/regress/sql/update.sql +++ b/src/test/regress/sql/update.sql @@ -124,11 +124,11 @@ INSERT INTO upsert_test VALUES (1, 'Bat'), (3, 'Zot') ON CONFLICT(a) INSERT INTO upsert_test VALUES (2, 'Beeble') ON CONFLICT(a) DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a) RETURNING tableoid::regclass, xmin = pg_current_xact_id()::xid AS xmin_correct, xmax = 0 AS xmax_correct; --- currently xmax is set after a conflict - that's probably not good, --- but it seems worthwhile to have to be explicit if that changes. +-- xmax is not carried forward after a conflict if the only prior locker +-- is the same transaction that is now updating the tuple. INSERT INTO upsert_test VALUES (2, 'Brox') ON CONFLICT(a) DO UPDATE SET (b, a) = (SELECT b || ', Excluded', a from upsert_test i WHERE i.a = excluded.a) - RETURNING tableoid::regclass, xmin = pg_current_xact_id()::xid AS xmin_correct, xmax = pg_current_xact_id()::xid AS xmax_correct; + RETURNING tableoid::regclass, xmin = pg_current_xact_id()::xid AS xmin_correct, xmax = 0 AS xmax_correct; DROP TABLE update_test; DROP TABLE upsert_test; -- 2.43.0