On Tue, Jul 28, 2026 at 05:44:15PM +0900, Michael Paquier wrote: > Hmm. Couldn't one say that it is an ordering problem with the xmax > computation in heap_update()? > > There is a window between the initial xmax computation and the TOAST > and/or page extension work where we could have concurrent updates, due > to the buffer lock getting released while we work on the TOAST data. > Concurrent transactions are then free to manipulation states, and the > xmax information gets overwritten in an incorrect way because it is > only written *before* touching TOAST. We could recompute it after the > TOAST/extension part is done, when the buffer lock is acquired again, > I guess? This way we would be able to keep track of the FK locks that > may have been added in-between. > > While looking at moving these blocks, I am not completely sure yet > about the part with locker_remains and checked_lockers. I'd assume > that we should recheck the xmax information with a > xmax_infomask_changed() and a RawXmax(), or something like that..
Attached is an idea of the manipulations involved, reworking the update flow as follows: - Move the Xmax compulation to be after any TOAST work, set it to InvalidTransactionId. The Cmin/Xmin is still done before. - Release buffer lock. - Do the TOAST work. - Reacquire buffer lock. - If we know that something has happened for TOAST and that the xmax or t_infomask has changed, a concurrent locker has come around. Assume that the locker is still active, switch locker_remains = true. - Finish filling infomask[2] and the Xmax, like on HEAD, just later. There is also an isolation test that detects the FK violation. This code is tricky enough that way more than one pair of eyes should look at this approach. So feel free to comment. -- Michael
From b9c014abfac91b8d4c785de2daa16bb6b7d3cf1a Mon Sep 17 00:00:00 2001 From: Michael Paquier <[email protected]> Date: Wed, 29 Jul 2026 12:56:17 +0900 Subject: [PATCH] Fix loss of tuple lock during TOAST step in heap_update() heap_update() computes xmax_old_tuple and xmax_new_tuple (the values to stamp on the old and new tuple versions) before releasing the buffer lock for TOAST or page-extension work. During that window, a concurrent session can add a KEY SHARE lock to the tuple via heap_lock_tuple() (e.g. from a foreign-key check), creating a MultiXact. When heap_update() reacquires the buffer lock, it overwrites the tuple's xmax with the pre-computed (stale) values, silently discarding the concurrent locker's lock. The xmax_old_tuple/xmax_new_tuple computation are moved to be after the TOAST/page-extension block, where we hold the buffer lock again and the tuple's xmax reflects any concurrent lockers that arrived during the window that could lead to an overwrite of the data. --- src/backend/access/heap/heapam.c | 156 +++++++++++------- src/test/isolation/expected/fk-toast-lock.out | 12 ++ src/test/isolation/isolation_schedule | 1 + src/test/isolation/specs/fk-toast-lock.spec | 53 ++++++ 4 files changed, 162 insertions(+), 60 deletions(-) create mode 100644 src/test/isolation/expected/fk-toast-lock.out create mode 100644 src/test/isolation/specs/fk-toast-lock.spec diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c index 8b488cfd8f68..7cd5c5fe272f 100644 --- a/src/backend/access/heap/heapam.c +++ b/src/backend/access/heap/heapam.c @@ -3309,11 +3309,13 @@ heap_update(Relation relation, const ItemPointerData *otid, HeapTuple newtup, bool locker_remains; bool id_has_external = false; TransactionId xmax_new_tuple, - xmax_old_tuple; + xmax_old_tuple, + xmax_lock_old_tuple = InvalidTransactionId; uint16 infomask_old_tuple, infomask2_old_tuple, infomask_new_tuple, - infomask2_new_tuple; + infomask2_new_tuple, + infomask_lock_old_tuple = 0; Assert(ItemPointerIsValid(otid)); @@ -3750,66 +3752,20 @@ l2: /* Fill in transaction status data */ /* - * If the tuple we're updating is locked, we need to preserve the locking - * info in the old tuple's Xmax. Prepare a new Xmax value for this. - */ - compute_new_xmax_infomask(HeapTupleHeaderGetRawXmax(oldtup.t_data), - oldtup.t_data->t_infomask, - oldtup.t_data->t_infomask2, - xid, *lockmode, true, - &xmax_old_tuple, &infomask_old_tuple, - &infomask2_old_tuple); - - /* - * And also prepare an Xmax value for the new copy of the tuple. If there - * was no xmax previously, or there was one but all lockers are now gone, - * then use InvalidTransactionId; otherwise, get the xmax from the old - * tuple. (In rare cases that might also be InvalidTransactionId and yet - * not have the HEAP_XMAX_INVALID bit set; that's fine.) - */ - if ((oldtup.t_data->t_infomask & HEAP_XMAX_INVALID) || - HEAP_LOCKED_UPGRADED(oldtup.t_data->t_infomask) || - (checked_lockers && !locker_remains)) - xmax_new_tuple = InvalidTransactionId; - else - xmax_new_tuple = HeapTupleHeaderGetRawXmax(oldtup.t_data); - - if (!TransactionIdIsValid(xmax_new_tuple)) - { - infomask_new_tuple = HEAP_XMAX_INVALID; - infomask2_new_tuple = 0; - } - else - { - /* - * If we found a valid Xmax for the new tuple, then the infomask bits - * to use on the new tuple depend on what was there on the old one. - * Note that since we're doing an update, the only possibility is that - * the lockers had FOR KEY SHARE lock. - */ - if (oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) - { - GetMultiXactIdHintBits(xmax_new_tuple, &infomask_new_tuple, - &infomask2_new_tuple); - } - else - { - infomask_new_tuple = HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY; - infomask2_new_tuple = 0; - } - } - - /* - * Prepare the new tuple with the appropriate initial values of Xmin and - * Xmax, as well as initial infomask bits as computed above. + * Prepare the new tuple with initial values of Xmin and Cmin, as well as + * initial infomask bits as computed above. + * + * Xmax and infomask for lockers are updated after the TOAST and/or page + * extension work, as releasing the buffer lock for TOAST allows + * concurrent transactions to add tuple locks that we must incorporate. + * See below for more details. */ newtup->t_data->t_infomask &= ~(HEAP_XACT_MASK); newtup->t_data->t_infomask2 &= ~(HEAP2_XACT_MASK); HeapTupleHeaderSetXmin(newtup->t_data, xid); HeapTupleHeaderSetCmin(newtup->t_data, cid); - newtup->t_data->t_infomask |= HEAP_UPDATED | infomask_new_tuple; - newtup->t_data->t_infomask2 |= infomask2_new_tuple; - HeapTupleHeaderSetXmax(newtup->t_data, xmax_new_tuple); + newtup->t_data->t_infomask |= HEAP_UPDATED | HEAP_XMAX_INVALID; + HeapTupleHeaderSetXmax(newtup->t_data, InvalidTransactionId); /* * Replace cid with a combo CID if necessary. Note that we already put @@ -3847,9 +3803,7 @@ l2: if (need_toast || newtupsize > pagefree) { - TransactionId xmax_lock_old_tuple; - uint16 infomask_lock_old_tuple, - infomask2_lock_old_tuple; + uint16 infomask2_lock_old_tuple; bool cleared_all_frozen = false; /* @@ -4037,6 +3991,88 @@ l2: newpage = BufferGetPage(newbuf); + /* + * Now compute the final Xmax values for both old and new tuples. We + * intentionally do this *after* the TOAST and/or page-extension work, as + * concurrent transactions may have added tuple locks, modifying the + * tuple's xmax. + */ + if (heaptup != newtup || newbuf != buffer) + { + /* + * If the tuple's xmax/infomask changed from the temporary lock we + * wrote before releasing the buffer lock, a concurrent locker must + * have arrived during TOAST. We compare the tuple's current Xmax + * and infomask with the values saved before the buffer was unlocked. + * If these values have not changed, the pre-TOAST checked_lockers + * and locker_remains are still valid. If these values differ, a + * new locker has arrived: assume that the locker is still active so + * that its lock is preserved in the new tuple's Xmax. + */ + if (xmax_infomask_changed(oldtup.t_data->t_infomask, + infomask_lock_old_tuple) || + !TransactionIdEquals(HeapTupleHeaderGetRawXmax(oldtup.t_data), + xmax_lock_old_tuple)) + { + checked_lockers = true; + locker_remains = true; + } + } + compute_new_xmax_infomask(HeapTupleHeaderGetRawXmax(oldtup.t_data), + oldtup.t_data->t_infomask, + oldtup.t_data->t_infomask2, + xid, *lockmode, true, + &xmax_old_tuple, &infomask_old_tuple, + &infomask2_old_tuple); + + /* + * Also prepare an Xmax value for the new copy of the tuple. If there was + * no xmax previously, or there was one but all lockers are now gone, then + * use InvalidTransactionId; otherwise, get the xmax from the old tuple. + * (In rare cases that might also be InvalidTransactionId and yet not have + * the HEAP_XMAX_INVALID bit set; that's fine.) + */ + if ((oldtup.t_data->t_infomask & HEAP_XMAX_INVALID) || + HEAP_LOCKED_UPGRADED(oldtup.t_data->t_infomask) || + (checked_lockers && !locker_remains)) + xmax_new_tuple = InvalidTransactionId; + else + xmax_new_tuple = HeapTupleHeaderGetRawXmax(oldtup.t_data); + + if (!TransactionIdIsValid(xmax_new_tuple)) + { + infomask_new_tuple = HEAP_XMAX_INVALID; + infomask2_new_tuple = 0; + } + else + { + /* + * If we found a valid Xmax for the new tuple, then the infomask bits + * to use on the new tuple depend on what was there on the old one. + * Note that since we're doing an update, the only possibility is that + * the lockers had FOR KEY SHARE lock. + */ + if (oldtup.t_data->t_infomask & HEAP_XMAX_IS_MULTI) + { + GetMultiXactIdHintBits(xmax_new_tuple, &infomask_new_tuple, + &infomask2_new_tuple); + } + else + { + infomask_new_tuple = HEAP_XMAX_KEYSHR_LOCK | HEAP_XMAX_LOCK_ONLY; + infomask2_new_tuple = 0; + } + } + + /* + * Set the new tuple's Xmax and associated infomask bits. + */ + heaptup->t_data->t_infomask &= ~(HEAP_XMAX_BITS); + heaptup->t_data->t_infomask2 &= ~(HEAP_KEYS_UPDATED); + heaptup->t_data->t_infomask |= infomask_new_tuple; + heaptup->t_data->t_infomask2 |= infomask2_new_tuple; + HeapTupleHeaderSetXmax(heaptup->t_data, xmax_new_tuple); + /* * We're about to do the actual update -- check for conflict first, to * avoid possibly having to roll back work we've just done. diff --git a/src/test/isolation/expected/fk-toast-lock.out b/src/test/isolation/expected/fk-toast-lock.out new file mode 100644 index 000000000000..28b563c5b275 --- /dev/null +++ b/src/test/isolation/expected/fk-toast-lock.out @@ -0,0 +1,12 @@ +Parsed test spec with 4 sessions + +starting permutation: s1_reindex s2_update s3_insert s1_commit s4_delete s3_commit +step s1_reindex: BEGIN; REINDEX TABLE pg_toast.pk_toast_toast; +step s2_update: UPDATE pk_toast SET payload = repeat('x', 10000) WHERE id = 1; <waiting ...> +step s3_insert: BEGIN; INSERT INTO fk_child(pid) VALUES (1); +step s1_commit: COMMIT; +step s2_update: <... completed> +step s4_delete: DELETE FROM pk_toast WHERE id = 1; <waiting ...> +step s3_commit: COMMIT; +step s4_delete: <... completed> +ERROR: update or delete on table "pk_toast" violates foreign key constraint "fk_child_pid_fkey" on table "fk_child" diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 26abed9f9f07..63f076b0b853 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -38,6 +38,7 @@ test: fk-snapshot test: fk-snapshot-2 test: fk-snapshot-3 test: fk-concurrent-pk-upd +test: fk-toast-lock test: subxid-overflow test: eval-plan-qual test: eval-plan-qual-trigger diff --git a/src/test/isolation/specs/fk-toast-lock.spec b/src/test/isolation/specs/fk-toast-lock.spec new file mode 100644 index 000000000000..fe036efe5c5a --- /dev/null +++ b/src/test/isolation/specs/fk-toast-lock.spec @@ -0,0 +1,53 @@ +# Test that a KEY SHARE lock taken by an foreign key check is not lost +# when a concurrent non-key UPDATE is inside its TOAST step. + +setup +{ + CREATE TABLE pk_toast ( + id int PRIMARY KEY, + payload text + ); + ALTER TABLE pk_toast ALTER COLUMN payload SET STORAGE EXTERNAL; + + CREATE TABLE fk_child ( + cid serial PRIMARY KEY, + pid int REFERENCES pk_toast(id) + ); + + INSERT INTO pk_toast VALUES (1, 'small'); + + -- Rename the TOAST table to a deterministic name. + SET allow_system_table_mods TO true; + DO $$DECLARE r record; + BEGIN + SELECT INTO r reltoastrelid::regclass::text AS table_name FROM pg_class + WHERE oid = 'pk_toast'::regclass; + EXECUTE 'ALTER TABLE ' || r.table_name || ' RENAME TO pk_toast_toast;'; + END$$; + RESET allow_system_table_mods; +} + +teardown +{ + DROP TABLE fk_child, pk_toast; +} + +# REINDEX blocking TOAST inserts. +session s1 +step s1_reindex { BEGIN; REINDEX TABLE pg_toast.pk_toast_toast; } +step s1_commit { COMMIT; } + +# Non-key UPDATE requiring TOAST. +session s2 +step s2_update { UPDATE pk_toast SET payload = repeat('x', 10000) WHERE id = 1; } + +# Insert a child row, foreign-key check taking KEY SHARE on parent. +session s3 +step s3_insert { BEGIN; INSERT INTO fk_child(pid) VALUES (1); } +step s3_commit { COMMIT; } + +# Delete parent row, blocked on KEY SHARE due to s3. +session s4 +step s4_delete { DELETE FROM pk_toast WHERE id = 1; } + +permutation s1_reindex s2_update s3_insert s1_commit s4_delete s3_commit -- 2.55.0
signature.asc
Description: PGP signature
