From fd95d3fc9136800ae2018425806be682cc36ab53 Mon Sep 17 00:00:00 2001 From: Amit Langote Date: Thu, 6 Aug 2026 21:44:03 +0900 Subject: [PATCH v1] Fix cross-type foreign keys in the batched fast-path FK check ri_FastPathFlushArray() rechecked a concurrently updated PK tuple with a scan key it built itself, putting found_val, the key of the tuple it had just locked, into sk_argument, and passing that same slot to recheck_matched_pk_tuple(). Both operands therefore came from the locked tuple, and since sk_argument is the operator's right-hand input, the PK value was read as an FK value. For a foreign key using a cross-type equality operator, such as a "date" primary key referenced by a "timestamp" column, that compares days against microseconds, so the recheck always failed and a batch that had to follow an update chain reported a violation even though the version it locked still had the key. Remove the recheck. For a same-type key it compared the tuple against itself and so never rejected anything, which was harmless only because the loop a few lines below does the real work: it already compares found_val, read after the chain has been followed, against every buffered FK value, with the arguments in the order the operator expects. That makes the recheck redundant as well as wrong. Detection is not weakened by dropping it, since the buffered value that led the scan to a tuple can be matched by no other row visible to our snapshot. ri_FastPathProbeOne() passes its original scan key, with the FK value still in sk_argument, and was never affected; nor were single-row statements or multi-column foreign keys, which go through it. Add an isolation test covering the cross-type case, a permutation where the key really does move away, and a same-type permutation that should behave identically. Reported-by: Peter Geoghegan Co-authored-by: Peter Geoghegan Discussion: https://postgr.es/m/CAH2-WznQjX3GByh_Ju7unuzMcik_5PJ5D7i_=qhwk=gPEkhfVQ@mail.gmail.com Backpatch-through: 19 --- src/backend/utils/adt/ri_triggers.c | 43 +++++++-------- .../expected/fk-crosstype-recheck.out | 37 +++++++++++++ src/test/isolation/isolation_schedule | 1 + .../isolation/specs/fk-crosstype-recheck.spec | 54 +++++++++++++++++++ 4 files changed, 111 insertions(+), 24 deletions(-) create mode 100644 src/test/isolation/expected/fk-crosstype-recheck.out create mode 100644 src/test/isolation/specs/fk-crosstype-recheck.spec diff --git a/src/backend/utils/adt/ri_triggers.c b/src/backend/utils/adt/ri_triggers.c index 627a9fb38ea..554b58ed06e 100644 --- a/src/backend/utils/adt/ri_triggers.c +++ b/src/backend/utils/adt/ri_triggers.c @@ -3178,32 +3178,24 @@ ri_FastPathFlushArray(RI_FastPathEntry *fpentry, TupleTableSlot *fk_slot, { Datum found_val; bool found_null; - bool concurrently_updated; - ScanKeyData recheck_skey[1]; - if (!ri_LockPKTuple(pk_rel, pk_slot, snapshot, &concurrently_updated)) + /* + * No key recheck is needed here, so we have no use for + * concurrently_updated. Unlike ri_FastPathProbeOne(), which takes + * the index scan's word for it that the tuple matches, this path + * compares the key against every buffered FK value below, and it does + * so using found_val, which is read out of the version we actually + * locked. A concurrent key update is therefore caught by that + * comparison: the batch item that led us to this tuple is left + * unmatched and reported as a violation. + */ + if (!ri_LockPKTuple(pk_rel, pk_slot, snapshot, NULL)) continue; /* Extract the PK value from the matched and locked tuple */ found_val = slot_getattr(pk_slot, riinfo->pk_attnums[0], &found_null); Assert(!found_null); - if (concurrently_updated) - { - /* - * Build a single-key scankey for recheck. We need the actual PK - * value that was found, not the FK search value. - */ - ScanKeyEntryInitialize(&recheck_skey[0], 0, 1, - fpmeta->strats[0], - fpmeta->subtypes[0], - idx_rel->rd_indcollation[0], - fpmeta->regops[0], - found_val); - if (!recheck_matched_pk_tuple(idx_rel, recheck_skey, 1, pk_slot)) - continue; - } - /* * Linear scan to mark all batch items matching this PK value. * O(batch_size) per match, O(batch_size^2) worst case -- fine for the @@ -3271,9 +3263,11 @@ ri_FastPathProbeOne(Relation pk_rel, Relation idx_rel, * Calls table_tuple_lock() directly with handling specific to RI checks. * Returns true if the tuple was successfully locked. * - * Sets *concurrently_updated to true if the locked tuple was reached - * by following an update chain (tmfd.traversed), indicating the caller - * should recheck the key. + * If concurrently_updated is not NULL, sets *concurrently_updated to true + * if the locked tuple was reached by following an update chain + * (tmfd.traversed), indicating the caller should recheck the key. Callers + * that compare the locked tuple's key against the value they were looking + * for anyway can pass NULL. */ static bool ri_LockPKTuple(Relation pk_rel, TupleTableSlot *slot, Snapshot snap, @@ -3283,7 +3277,8 @@ ri_LockPKTuple(Relation pk_rel, TupleTableSlot *slot, Snapshot snap, TM_Result result; int lockflags = TUPLE_LOCK_FLAG_LOCK_UPDATE_IN_PROGRESS; - *concurrently_updated = false; + if (concurrently_updated) + *concurrently_updated = false; if (!IsolationUsesXactSnapshot()) lockflags |= TUPLE_LOCK_FLAG_FIND_LAST_VERSION; @@ -3296,7 +3291,7 @@ ri_LockPKTuple(Relation pk_rel, TupleTableSlot *slot, Snapshot snap, switch (result) { case TM_Ok: - if (tmfd.traversed) + if (tmfd.traversed && concurrently_updated) *concurrently_updated = true; return true; diff --git a/src/test/isolation/expected/fk-crosstype-recheck.out b/src/test/isolation/expected/fk-crosstype-recheck.out new file mode 100644 index 00000000000..875dc856bce --- /dev/null +++ b/src/test/isolation/expected/fk-crosstype-recheck.out @@ -0,0 +1,37 @@ +Parsed test spec with 2 sessions + +starting permutation: s1b s1away s1back s2ins s1c s2sel +step s1b: BEGIN; +step s1away: UPDATE fkct_pk SET k = '2020-06-01' WHERE payload = 'p1'; +step s1back: UPDATE fkct_pk SET k = '2020-01-01' WHERE payload = 'p1'; +step s2ins: INSERT INTO fkct_fk SELECT g, '2020-01-01'::timestamp FROM generate_series(1,2) g; +step s1c: COMMIT; +step s2ins: <... completed> +step s2sel: SELECT k FROM fkct_pk; + k +---------- +01-01-2020 +(1 row) + + +starting permutation: s1b s1away s2ins s1c s2sel +step s1b: BEGIN; +step s1away: UPDATE fkct_pk SET k = '2020-06-01' WHERE payload = 'p1'; +step s2ins: INSERT INTO fkct_fk SELECT g, '2020-01-01'::timestamp FROM generate_series(1,2) g; +step s1c: COMMIT; +step s2ins: <... completed> +ERROR: insert or update on table "fkct_fk" violates foreign key constraint "fkct_fk_t_fkey" +step s2sel: SELECT k FROM fkct_pk; + k +---------- +06-01-2020 +(1 row) + + +starting permutation: s1b s1aways s1backs s2inss s1c +step s1b: BEGIN; +step s1aways: UPDATE fkct_pk_same SET k = '2020-06-01' WHERE payload = 'p1'; +step s1backs: UPDATE fkct_pk_same SET k = '2020-01-01' WHERE payload = 'p1'; +step s2inss: INSERT INTO fkct_fk_same SELECT g, '2020-01-01'::timestamp FROM generate_series(1,2) g; +step s1c: COMMIT; +step s2inss: <... completed> diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index df8ce44ede6..ad2042ced69 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -30,6 +30,7 @@ test: detach-partition-concurrently-2 test: detach-partition-concurrently-3 test: detach-partition-concurrently-4 test: fk-contention +test: fk-crosstype-recheck test: fk-deadlock test: fk-deadlock2 test: fk-partitioned-1 diff --git a/src/test/isolation/specs/fk-crosstype-recheck.spec b/src/test/isolation/specs/fk-crosstype-recheck.spec new file mode 100644 index 00000000000..5d479b8d46c --- /dev/null +++ b/src/test/isolation/specs/fk-crosstype-recheck.spec @@ -0,0 +1,54 @@ +# A foreign key may use a cross-type equality operator: a "date" primary key +# and a "timestamp" referencing column give "=(date,timestamp without time +# zone)", whose left input is the PK type and whose right input is the FK type. +# +# When the referenced row is updated while a check is locking it, the check +# has to re-check against the new version of the row. That re-check must +# still pass each value to the side of the operator that expects it. A date +# counts days and a timestamp counts microseconds, so reading one as the other +# compares two unrelated numbers. Both types are pass-by-value, so nothing +# here turns on how a value is stored, only on which side it is read from. +# +# Below the referenced key is present the whole time -- s1 moves it away and +# puts it back inside one transaction -- so the INSERT must succeed, exactly as +# it does for the same-type case in the second permutation. + +setup +{ + CREATE TABLE fkct_pk (k date PRIMARY KEY, payload text); + CREATE TABLE fkct_fk (id int, t timestamp REFERENCES fkct_pk(k)); + INSERT INTO fkct_pk VALUES ('2020-01-01', 'p1'); + + CREATE TABLE fkct_pk_same (k timestamp PRIMARY KEY, payload text); + CREATE TABLE fkct_fk_same (id int, t timestamp REFERENCES fkct_pk_same(k)); + INSERT INTO fkct_pk_same VALUES ('2020-01-01', 'p1'); +} + +teardown +{ + DROP TABLE fkct_fk, fkct_pk, fkct_fk_same, fkct_pk_same; +} + +session s1 +step s1b { BEGIN; } +step s1away { UPDATE fkct_pk SET k = '2020-06-01' WHERE payload = 'p1'; } +step s1back { UPDATE fkct_pk SET k = '2020-01-01' WHERE payload = 'p1'; } +step s1aways { UPDATE fkct_pk_same SET k = '2020-06-01' WHERE payload = 'p1'; } +step s1backs { UPDATE fkct_pk_same SET k = '2020-01-01' WHERE payload = 'p1'; } +step s1c { COMMIT; } + +# Two rows in one statement, so the checks are batched -- that is what reaches +# the re-check path under test. +session s2 +step s2ins { INSERT INTO fkct_fk SELECT g, '2020-01-01'::timestamp FROM generate_series(1,2) g; } +step s2inss { INSERT INTO fkct_fk_same SELECT g, '2020-01-01'::timestamp FROM generate_series(1,2) g; } +step s2sel { SELECT k FROM fkct_pk; } + +permutation s1b s1away s1back s2ins s1c s2sel + +# The mirror image: s1 leaves the key where it moved it, so the INSERT must +# fail. Making the re-check accept every concurrently updated tuple would +# satisfy the permutation above while breaking this one. +permutation s1b s1away s2ins s1c s2sel + +permutation s1b s1aways s1backs s2inss s1c -- 2.47.3