From 91839b1130da7e62781224cec33c0a47a6995968 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Mon, 31 Aug 2026 22:45:08 +0900 Subject: [PATCH v1] Take the RI fast-path snapshot after locking the PK relation ri_FastPathCheck() registers the snapshot its index scan will use before it calls table_open() on the referenced relation. Waiting for that lock can take arbitrarily long, and a transaction that commits during the wait is invisible to a snapshot taken before it. If what commits is the referenced row, the scan does not find it and the check reports a foreign key violation for a key that exists. SPI does not have this problem: RI_FKey_check() passes detectNewRows = false, so ri_PerformCheck() passes InvalidSnapshot and SPI takes its snapshot when the plan executes, after the executor has taken its locks. The fast path collapsed that into one early GetTransactionSnapshot(). ri_fastpath_reindex already covers this window: its second permutation commits the referenced row while the check is parked at ri-before-pk-lock. It passes today because the cases it exercises are ordinary DML, which is batched, and so reach ri_FastPathBatchFlush(); that takes its snapshot at flush time, after ri_FastPathGetEntry() has opened the relations. Forcing the dispatch in RI_FKey_check() to take the per-row path rather than the batched one makes the permutation fail. Move the snapshot acquisition after table_open() and the constraint re-read. While at it, add a few permutations to fk-snapshot-2.spec covering a referenced row committed after the referencing transaction's snapshot. The results differ by isolation level, so they are sensitive to which snapshot the check uses. Discussion: https://postgr.es/m/ Backpatch-though: 19 --- src/backend/utils/adt/ri_triggers.c | 14 +++++- src/test/isolation/expected/fk-snapshot-2.out | 44 +++++++++++++++++++ src/test/isolation/specs/fk-snapshot-2.spec | 15 +++++++ 3 files changed, 72 insertions(+), 1 deletion(-) diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index d4545618634..9661b9d5f38 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -2882,7 +2882,6 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo, * ri_PerformCheck(). */ CommandCounterIncrement(); - snapshot = RegisterSnapshot(GetTransactionSnapshot()); INJECTION_POINT("ri-before-pk-lock", NULL); @@ -2893,6 +2892,19 @@ ri_FastPathCheck(RI_ConstraintInfo *riinfo, idx_rel = index_open(riinfo->conindid, AccessShareLock); + /* + * Only now take the snapshot the scan will use. Acquiring it before + * table_open() would let an unbounded amount of time pass while we wait + * for the lock, during which another transaction can commit the very row + * we are about to look for. The scan would not see it and the check + * would report a violation for a key that exists. + * + * The SPI path does not have this problem: for this check it passes + * InvalidSnapshot, so SPI takes the snapshot when the plan is executed, + * after the executor has taken its locks. + */ + snapshot = RegisterSnapshot(GetTransactionSnapshot()); + slot = table_slot_create(pk_rel, NULL); GetUserIdAndSecContext(&saved_userid, &saved_sec_context); diff --git a/src/test/isolation/expected/fk-snapshot-2.out b/src/test/isolation/expected/fk-snapshot-2.out index 7333643e9ac..376460a00e5 100644 --- a/src/test/isolation/expected/fk-snapshot-2.out +++ b/src/test/isolation/expected/fk-snapshot-2.out @@ -59,3 +59,47 @@ step s1c: COMMIT; step s2ins: <... completed> ERROR: could not serialize access due to concurrent delete step s2c: COMMIT; + +starting permutation: s1rr s2rr s2sel s1ins s1c s2ins2 s2c +step s1rr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2rr: BEGIN ISOLATION LEVEL REPEATABLE READ; +step s2sel: SELECT count(*) FROM parent; +count +----- + 1 +(1 row) + +step s1ins: INSERT INTO parent VALUES (2); +step s1c: COMMIT; +step s2ins2: INSERT INTO child VALUES (2, 2); +ERROR: insert or update on table "child" violates foreign key constraint "child_parent_id_fkey" +step s2c: COMMIT; + +starting permutation: s1ser s2ser s2sel s1ins s1c s2ins2 s2c +step s1ser: BEGIN ISOLATION LEVEL SERIALIZABLE; +step s2ser: BEGIN ISOLATION LEVEL SERIALIZABLE; +step s2sel: SELECT count(*) FROM parent; +count +----- + 1 +(1 row) + +step s1ins: INSERT INTO parent VALUES (2); +step s1c: COMMIT; +step s2ins2: INSERT INTO child VALUES (2, 2); +ERROR: insert or update on table "child" violates foreign key constraint "child_parent_id_fkey" +step s2c: COMMIT; + +starting permutation: s1rc s2rc s2sel s1ins s1c s2ins2 s2c +step s1rc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2rc: BEGIN ISOLATION LEVEL READ COMMITTED; +step s2sel: SELECT count(*) FROM parent; +count +----- + 1 +(1 row) + +step s1ins: INSERT INTO parent VALUES (2); +step s1c: COMMIT; +step s2ins2: INSERT INTO child VALUES (2, 2); +step s2c: COMMIT; diff --git a/src/test/isolation/specs/fk-snapshot-2.spec b/src/test/isolation/specs/fk-snapshot-2.spec index 94cd151aab9..5b0144c0f70 100644 --- a/src/test/isolation/specs/fk-snapshot-2.spec +++ b/src/test/isolation/specs/fk-snapshot-2.spec @@ -21,6 +21,7 @@ step s1rr { BEGIN ISOLATION LEVEL REPEATABLE READ; } step s1ser { BEGIN ISOLATION LEVEL SERIALIZABLE; } step s1del { DELETE FROM parent WHERE parent_id = 1; } step s1c { COMMIT; } +step s1ins { INSERT INTO parent VALUES (2); } session s2 step s2rc { BEGIN ISOLATION LEVEL READ COMMITTED; } @@ -28,6 +29,8 @@ step s2rr { BEGIN ISOLATION LEVEL REPEATABLE READ; } step s2ser { BEGIN ISOLATION LEVEL SERIALIZABLE; } step s2ins { INSERT INTO child VALUES (1, 1); } step s2c { COMMIT; } +step s2sel { SELECT count(*) FROM parent; } +step s2ins2 { INSERT INTO child VALUES (2, 2); } # Violates referential integrity unless we use a crosscheck snapshot, # which is up-to-date compared with the transaction's snapshot. @@ -48,3 +51,15 @@ permutation s1ser s2ser s2ins s1del s2c s1c # We raise a concurrent update error # which is good enough: permutation s1ser s2ser s1del s2ins s1c s2c + +# A parent row committed by another transaction after this one took its +# snapshot. RI_FKey_check passes detectNewRows = false, so the check runs +# under the transaction snapshot rather than a current one, and the row is +# correctly not visible: referencing it is a violation. Only the parent-side +# checks need to see rows committed since the snapshot. +permutation s1rr s2rr s2sel s1ins s1c s2ins2 s2c +permutation s1ser s2ser s2sel s1ins s1c s2ins2 s2c + +# The same order in READ COMMITTED, where the check's snapshot is a fresh one +# and the parent row is visible. +permutation s1rc s2rc s2sel s1ins s1c s2ins2 s2c -- 2.47.3