Re: lost lock during toasting allows fk violation

Hi Michael,

Recomputing after the toast work is the right direction, but the orphan is
still reachable with v1 applied: the recheck is gated on a different
condition than the one that dropped the buffer lock, and what it recomputes
from carries the lock we took ourselves. 0001 fixes both, 0002 is the
injection point and the isolation test for the first. Both apply on top of
v1. fk-toast-lock itself fails on master at b4dfae2ffac and passes on v1.

1. The recheck is not gated on the condition that dropped the buffer lock.

if (heaptup != newtup || newbuf != buffer)

vs.

if (need_toast || newtupsize > pagefree)

need_toast is true whenever the old tuple has an external value, so an
update of an unrelated column enters the block, writes the temporary lock
and releases the buffer lock. If the toaster then has nothing to do,
heap_toast_insert_or_update() hands back its input:

else
result_tuple = newtup;

and if the new tuple still fits on the page, heaptup == newtup and
newbuf == buffer, so the recheck is skipped on a path that did release the
lock.

Skipping it changes the outcome when checked_lockers is set and
locker_remains is not:

XactLockTableWait(xwait, relation, &oldtup.t_self,
XLTW_Update);
checked_lockers = true;

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;

Repro on your pk_toast/fk_child, with a column to update and a payload that
is already external before the update starts. Nothing in the window is
blockable from SQL once the toaster has no work to do, so this uses the
injection point 0002 adds right after the buffer lock is dropped:

CREATE EXTENSION injection_points;

CREATE TABLE pk_toast (id int PRIMARY KEY, n int, 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, 0, repeat('x', 10000));

s2: SELECT FROM injection_points_set_local();
SELECT FROM injection_points_attach('heap_update-toast-unlocked', 'wait');

s1: BEGIN; SELECT id FROM pk_toast WHERE id = 1 FOR NO KEY UPDATE;

s2: UPDATE pk_toast SET n = n + 1 WHERE id = 1;
-- waits on s1, Lock/transactionid

s1: COMMIT;
-- s2 resumes and stops in the window,
-- InjectionPoint/heap_update-toast-unlocked

s3: BEGIN; INSERT INTO fk_child(pid) VALUES (1);
-- the foreign key check takes KEY SHARE on the parent

s1: SELECT FROM injection_points_detach('heap_update-toast-unlocked');
SELECT FROM injection_points_wakeup('heap_update-toast-unlocked');
-- s2 finishes and commits

s4: DELETE FROM pk_toast WHERE id = 1;
s3: COMMIT;

s1 is what leaves heap_update() with checked_lockers set and locker_remains
clear: its lock is gone by the time the update gets past
XactLockTableWait(). s4 has to run after s2 has committed, otherwise it
blocks on the old tuple's Xmax and only reaches the new version once s3 has
committed too, which hides the lost lock -- fk-toast-lock has that ordering
right already.

v1: s4 does not wait. The parent row is deleted and fk_child
keeps its row: 0 parents, 1 child, 1 orphan.
v1 + 0001: s4 waits on Lock/transactionid, and s3's commit turns that into
ERROR: update or delete on table "pk_toast" violates foreign
key constraint "fk_child_pid_fkey" on table "fk_child"

0002 is that as an isolation test. On v1 it fails the same way
fk-toast-lock does on master.

2. With nobody else around, the new version comes out locked by the updating
transaction itself.

CREATE TABLE tq (id int PRIMARY KEY, n int, payload text);
ALTER TABLE tq ALTER COLUMN payload SET STORAGE EXTERNAL;
INSERT INTO tq VALUES (1, 0, repeat('x', 10000));
UPDATE tq SET n = n + 1 WHERE id = 1 RETURNING xmax;

master: xmax = 0
v1: xmax = 698 -- 698 is the UPDATE's own xid

SELECT lp, t_xmin, t_xmax, t_infomask::bit(16) AS infomask
FROM heap_page_items(get_raw_page('tq', 0));

master:
lp | t_xmin | t_xmax | infomask
----+--------+--------+------------------
1 | 697 | 698 | 0000000100000110
2 | 698 | 0 | 0010100000000110

v1:
lp | t_xmin | t_xmax | infomask
----+--------+--------+------------------
1 | 697 | 698 | 0000000100000110
2 | 698 | 698 | 0010000010010110

lp 2 is the new version, and its lock bits are HEAP_XMAX_LOCK_ONLY |
HEAP_XMAX_KEYSHR_LOCK. Same else branch as above: HEAP_XMAX_INVALID has
been cleared by the temporary lock. That same Xmax is what the old tuple's
final one is computed from, which is point 3. And it makes this comment
wrong: the lock those bits are taken from is our own fornokeyupd one, not a
key share lock at all.

* Note that since we're doing an update, the only possibility is that
* the lockers had FOR KEY SHARE lock.

RETURNING only shows it when the toaster hands its input tuple back. When
the toaster makes a copy the page is stamped just the same, but v1 sets the
Xmax on that copy alone, so the caller's tuple keeps the placeholder
HEAP_XMAX_INVALID and no longer agrees with what was stored. HEAD set it
before the toaster ran, so the two always agreed.

3. With a real key-share locker, the updater lands in the multixact twice.

-- mq is tq again
s1: BEGIN; SELECT id FROM mq WHERE id = 1 FOR KEY SHARE;
s2: UPDATE mq SET payload = repeat('x',10000) WHERE id = 1;

master: lp1 multi 1: 704 keysh, 705 nokeyupd
lp2 xmax = 704, plain xid, KEYSHR|LOCK_ONLY
v1: lp1 multi 2: 704 keysh, 705 fornokeyupd, 705 nokeyupd
lp2 multi 1: 704 keysh, 705 fornokeyupd
v1 + 0001: lp1 multi 2: 704 keysh, 705 nokeyupd
lp2 xmax = 704, plain xid, KEYSHR|LOCK_ONLY

705 is the updater. MultiXactIdExpand() appending rather than replacing a
member with the same xid is not new -- master reaches that on a plain lock
upgrade. What is new is the update path reaching it, because xmax_old_tuple
is now computed from an Xmax that already carries our own temporary lock.

0001 keeps the Xmax/infomask from before that lock and computes from those
when nothing else touched the tuple meanwhile; when a locker did arrive, it
takes our own lock back out of the Xmax instead, so that the computations
see the Xmax as if we had never taken the lock -- which also makes the
comment above true again. Nothing is taken out when locking the tuple left
the Xmax alone, since MultiXactIdExpand() hands the multixact back unchanged
when the same transaction already holds that status, and then the entry is
not ours to remove. The release is tracked with a flag rather than
inferred, and the final Xmax is written to the caller's tuple as well.

The same case as point 3 with one thing changed: the locker takes its KEY
SHARE in the middle of the update, parked in the window, rather than before
it starts. Not after it either -- that just locks the new version and every
build agrees. 697 is the updater, 698 the locker:

master: lp1 xmax = 697, plain xid, no lock bits
lp2 xmax = 0 -- 698's lock is gone
v1: lp1 multi 2: 697 fornokeyupd, 697 nokeyupd, 698 keysh
lp2 multi 1: 697 fornokeyupd, 698 keysh
v1 + 0001: lp1 multi 2: 697 nokeyupd, 698 keysh
lp2 xmax = 698, plain xid, KEYSHR|LOCK_ONLY

Only the timing moved, and only master's answer moved with it: the lock is
simply gone. 0001 lands on the shape master produces with the locker there
all along, so the window stops making a difference. This is
also the case that exercises taking our own lock back out -- in point 3
nothing changes while the buffer is unlocked, so the Xmax from before the
lock is used as it stands. Taking it out costs a multixact only when more
than one locker is left behind; with a single one the Xmax goes back to a
plain xid, as here.

On the locker_remains part you were unsure about: with our own lock out of
the way there is nothing left to assume, since whether a locker remains is
read off the members just walked. The case left guessing is an Xmax our
lock cannot be removed from, and there erring towards locker_remains = true
is safe: worst case the locker is already gone and the new tuple carries a
lock-only Xmax of an ended xact, which waiters resolve against xact status
as usual.

make check 245/245, isolation including fk-toast-lock, injection_points
including the new spec, and check-world. Replaying the heap WAL of a run
full of TOASTing updates under wal_consistency_checking = 'heap' reports no
inconsistent pages on master, on v1 and with 0001.

Jacob mentioned back branches were not tested: the same ordering -- Xmax
computed before the toast block -- is in all supported branches, and dates
to 0ac5ad5134f.

One consequence of moving the computation down I could not get rid of: it
now runs after RelationGetBufferForTuple(), so compute_new_xmax_infomask()
can create a multixact while content locks on both buffer and newbuf are
held, where on HEAD only buffer was held. Nothing else is taken there, so
the lock order is unchanged, but it is your call whether that is fine.

By the way, 0001 also drops the "as computed above" from the comment on the
Xmin/Cmin block, since nothing is computed there any more.

Thanks,
Rui