Hi, > The root cause is that unique checks use a dirty snapshot. They can > decide a key is unused because of a deletion the transaction cannot > see. The fix might be to ask our own snapshot the same question, and > fail if it still sees the key in use. I don't think this will require > an ABI change.
I prepared a PoC patch that shows how this could be archived. Also it includes regression tests for everything discussed above. One drawback I discovered is that the patch introduces a false-positive conflict detection that causes the existing partition-key-update-3.spec to fail. I'm not 100% certain what to do about it. Given the fact that SSI generally has false-positives, maybe we should just rewrite the test? This seems to be better compared to what we currently have and personally I don't have better proposals at the moment. Thoughts? -- Best regards, Aleksander Alekseev
From e3f0df8affd2d938a096adeafe021c583950c395 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Thu, 8 Oct 2026 15:45:37 +0300 Subject: [PATCH v1] PoC bugfix by a.alekseev v1 https://postgr.es/m/CA%2BCOZaBOiiRPmEfX00oE%3DN6HBSZVe0Y-y-ZqqaXq8BAAj1gu%2BQ%40mail.gmail.com --- src/backend/access/nbtree/nbtinsert.c | 60 +++++++++++-------- src/backend/executor/execIndexing.c | 38 ++++++++++++ src/backend/storage/lmgr/predicate.c | 42 +++++++++++++ src/include/storage/predicate.h | 2 + .../expected/read-write-unique-5.out | 31 ++++++++++ .../expected/read-write-unique-6.out | 16 +++++ .../expected/read-write-unique-7.out | 21 +++++++ src/test/isolation/isolation_schedule | 3 + .../isolation/specs/read-write-unique-5.spec | 39 ++++++++++++ .../isolation/specs/read-write-unique-6.spec | 37 ++++++++++++ .../isolation/specs/read-write-unique-7.spec | 37 ++++++++++++ 11 files changed, 301 insertions(+), 25 deletions(-) create mode 100644 src/test/isolation/expected/read-write-unique-5.out create mode 100644 src/test/isolation/expected/read-write-unique-6.out create mode 100644 src/test/isolation/expected/read-write-unique-7.out create mode 100644 src/test/isolation/specs/read-write-unique-5.spec create mode 100644 src/test/isolation/specs/read-write-unique-6.spec create mode 100644 src/test/isolation/specs/read-write-unique-7.spec diff --git a/src/backend/access/nbtree/nbtinsert.c b/src/backend/access/nbtree/nbtinsert.c index d5d964301c1..550633a6374 100644 --- a/src/backend/access/nbtree/nbtinsert.c +++ b/src/backend/access/nbtree/nbtinsert.c @@ -27,6 +27,7 @@ #include "storage/lmgr.h" #include "storage/predicate.h" #include "utils/injection_point.h" +#include "utils/snapmgr.h" /* Minimum tree height for application of fastpath optimization */ #define BTREE_FASTPATH_MIN_LEVEL 2 @@ -674,36 +675,45 @@ _bt_check_unique(Relation rel, BTInsertState insertstate, Relation heapRel, RelationGetRelationName(rel)))); } } - else if (all_dead && (!inposting || - (prevalldead && - curposti == BTreeTupleGetNPosting(curitup) - 1))) + else { /* - * The conflicting tuple (or all HOT chains pointed to by - * all posting list TIDs) is dead to everyone, so try to - * mark the index entry killed. It's ok if we're not - * allowed to, this isn't required for correctness. + * The dirty snapshot found nothing, so the key looks + * unused. Make sure our own snapshot agrees. */ - Buffer buf; - - /* Be sure to operate on the proper buffer */ - if (nbuf != InvalidBuffer) - buf = nbuf; - else - buf = insertstate->buf; + CheckForSerializableKeyReuse(heapRel, &htid); - /* - * Use the hint bit infrastructure to check if we can - * update the page while just holding a share lock. - * - * Can't use BufferSetHintBits16() here as we update two - * different locations. - */ - if (BufferBeginSetHintBits(buf)) + if (all_dead && (!inposting || + (prevalldead && + curposti == BTreeTupleGetNPosting(curitup) - 1))) { - ItemIdMarkDead(curitemid); - opaque->btpo_flags |= BTP_HAS_GARBAGE; - BufferFinishSetHintBits(buf, true, true); + /* + * The conflicting tuple (or all HOT chains pointed to + * by all posting list TIDs) is dead to everyone, so try + * to mark the index entry killed. It's ok if we're not + * allowed to, this isn't required for correctness. + */ + Buffer buf; + + /* Be sure to operate on the proper buffer */ + if (nbuf != InvalidBuffer) + buf = nbuf; + else + buf = insertstate->buf; + + /* + * Use the hint bit infrastructure to check if we can + * update the page while just holding a share lock. + * + * Can't use BufferSetHintBits16() here as we update two + * different locations. + */ + if (BufferBeginSetHintBits(buf)) + { + ItemIdMarkDead(curitemid); + opaque->btpo_flags |= BTP_HAS_GARBAGE; + BufferFinishSetHintBits(buf, true, true); + } } } diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index d0a714a42f2..7590a762d79 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -114,6 +114,7 @@ #include "executor/executor.h" #include "nodes/nodeFuncs.h" #include "storage/lmgr.h" +#include "storage/predicate.h" #include "utils/injection_point.h" #include "utils/lsyscache.h" #include "utils/multirangetypes.h" @@ -969,6 +970,43 @@ retry: * we no longer complain if found_self is still false. */ + /* + * The dirty scan skipped rows it found dead, so repeat the search with the + * query snapshot and let CheckForSerializableKeyReuse() judge the outcome. + */ + if (!conflict && ActiveSnapshotSet() && + CheckForSerializableConflictOutNeeded(heap, GetActiveSnapshot())) + { + Datum existing_values[INDEX_MAX_KEYS]; + bool existing_isnull[INDEX_MAX_KEYS]; + + index_scan = index_beginscan(heap, index, false, + GetActiveSnapshot(), NULL, indnkeyatts, 0, + SO_NONE); + index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0); + + while (table_index_getnext_slot(index_scan, ForwardScanDirection, + existing_slot)) + { + if (ItemPointerIsValid(tupleid) && + ItemPointerEquals(tupleid, &existing_slot->tts_tid)) + continue; + + FormIndexDatum(indexInfo, existing_slot, estate, + existing_values, existing_isnull); + + if (index_scan->xs_recheck && + !index_recheck_constraint(index, constr_procs, + existing_values, existing_isnull, + values)) + continue; + + CheckForSerializableKeyReuse(heap, &existing_slot->tts_tid); + } + + index_endscan(index_scan); + } + econtext->ecxt_scantuple = save_scantuple; ExecDropSingleTupleTableSlot(existing_slot); diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index 0ae85b7d5b4..a93ded290a2 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -195,6 +195,7 @@ #include "access/parallel.h" #include "access/slru.h" +#include "access/tableam.h" #include "access/transam.h" #include "access/twophase.h" #include "access/twophase_rmgr.h" @@ -4419,6 +4420,47 @@ CheckTableForSerializableConflictIn(Relation relation) LWLockRelease(SerializablePredicateListLock); } +/* + * Refuse to let a uniqueness check conclude that a key is unused when our own + * snapshot says otherwise. + */ +void +CheckForSerializableKeyReuse(Relation relation, const ItemPointerData *tid) +{ + SnapshotData snap; + ItemPointerData htid; + Snapshot snapshot; + + if (!ActiveSnapshotSet()) + return; + snapshot = GetActiveSnapshot(); + + if (!CheckForSerializableConflictOutNeeded(relation, snapshot)) + return; + + htid = *tid; + + /* + * Consider all changes within the transaction and the current + * command visible. + */ + snap = *snapshot; + snap.curcid = InvalidCommandId; + + if (!table_fetch_tid(relation, &htid, &snap, NULL)) + return; + + LWLockAcquire(SerializableXactHashLock, LW_EXCLUSIVE); + MySerializableXact->flags |= SXACT_FLAG_DOOMED; + LWLockRelease(SerializableXactHashLock); + + ereport(ERROR, + (errcode(ERRCODE_T_R_SERIALIZATION_FAILURE), + errmsg("could not serialize access due to read/write dependencies among transactions"), + errdetail_internal("Reason code: Canceled on reuse of a key which a concurrent transaction deleted."), + errhint("The transaction might succeed if retried."))); +} + /* * Flag a rw-dependency between two serializable transactions. diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h index 443bffb58fd..f68571b1799 100644 --- a/src/include/storage/predicate.h +++ b/src/include/storage/predicate.h @@ -66,6 +66,8 @@ extern bool CheckForSerializableConflictOutNeeded(Relation relation, Snapshot sn extern void CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot snapshot); extern void CheckForSerializableConflictIn(Relation relation, const ItemPointerData *tid, BlockNumber blkno); extern void CheckTableForSerializableConflictIn(Relation relation); +extern void CheckForSerializableKeyReuse(Relation relation, + const ItemPointerData *tid); /* final rollback checking */ extern void PreCommit_CheckForSerializationFailure(void); diff --git a/src/test/isolation/expected/read-write-unique-5.out b/src/test/isolation/expected/read-write-unique-5.out new file mode 100644 index 00000000000..7b2f079fad4 --- /dev/null +++ b/src/test/isolation/expected/read-write-unique-5.out @@ -0,0 +1,31 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_1 s2_1 c2 s1_2a s1_3 c1 +step s1_1: SELECT k, j FROM test WHERE k = 1; +k| j +-+------- +1|1000000 +(1 row) + +step s2_1: DELETE FROM test WHERE j = 1000000; +step c2: COMMIT; +step s1_2a: INSERT INTO test VALUES (1, 2); +ERROR: could not serialize access due to read/write dependencies among transactions +step s1_3: SELECT k, j FROM test WHERE k = 1 ORDER BY j; +ERROR: current transaction is aborted, commands ignored until end of transaction block +step c1: COMMIT; + +starting permutation: s1_1 s2_1 c2 s1_2b s1_3 c1 +step s1_1: SELECT k, j FROM test WHERE k = 1; +k| j +-+------- +1|1000000 +(1 row) + +step s2_1: DELETE FROM test WHERE j = 1000000; +step c2: COMMIT; +step s1_2b: INSERT INTO test VALUES (1, 2) ON CONFLICT DO NOTHING; +ERROR: could not serialize access due to read/write dependencies among transactions +step s1_3: SELECT k, j FROM test WHERE k = 1 ORDER BY j; +ERROR: current transaction is aborted, commands ignored until end of transaction block +step c1: COMMIT; diff --git a/src/test/isolation/expected/read-write-unique-6.out b/src/test/isolation/expected/read-write-unique-6.out new file mode 100644 index 00000000000..094d3d20100 --- /dev/null +++ b/src/test/isolation/expected/read-write-unique-6.out @@ -0,0 +1,16 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_1 s2_1 c2 s1_2 s1_3 c1 +step s1_1: SELECT k, j FROM test WHERE k = 1; +k| j +-+------- +1|1000000 +(1 row) + +step s2_1: DELETE FROM test WHERE j = 1000000; +step c2: COMMIT; +step s1_2: INSERT INTO test VALUES (1, 2); +ERROR: could not serialize access due to read/write dependencies among transactions +step s1_3: SELECT k, j FROM test WHERE k = 1 ORDER BY j; +ERROR: current transaction is aborted, commands ignored until end of transaction block +step c1: COMMIT; diff --git a/src/test/isolation/expected/read-write-unique-7.out b/src/test/isolation/expected/read-write-unique-7.out new file mode 100644 index 00000000000..2e0c3cafc57 --- /dev/null +++ b/src/test/isolation/expected/read-write-unique-7.out @@ -0,0 +1,21 @@ +Parsed test spec with 2 sessions + +starting permutation: s1_1 s2_1 s1_2 c2 s1_3 c1 +step s1_1: SELECT k, j FROM test WHERE k = 1; +k| j +-+------- +1|1000000 +(1 row) + +step s2_1: DELETE FROM test WHERE j = 1000000; +step s1_2: INSERT INTO test VALUES (1, 2); +step c2: COMMIT; +step s1_3: SELECT k, j FROM test WHERE k = 1 ORDER BY j; +k| j +-+------- +1| 2 +1|1000000 +(2 rows) + +step c1: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index f1676a961f9..01cf9f059ef 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -5,6 +5,9 @@ test: read-write-unique test: read-write-unique-2 test: read-write-unique-3 test: read-write-unique-4 +test: read-write-unique-5 +test: read-write-unique-6 +test: read-write-unique-7 test: simple-write-skew test: receipt-report test: temporal-range-integrity diff --git a/src/test/isolation/specs/read-write-unique-5.spec b/src/test/isolation/specs/read-write-unique-5.spec new file mode 100644 index 00000000000..ac51ea3028f --- /dev/null +++ b/src/test/isolation/specs/read-write-unique-5.spec @@ -0,0 +1,39 @@ +# Reusing a key that another transaction has deleted and committed, under a +# primary key constraint, with a plain insert and with ON CONFLICT DO NOTHING. + +setup +{ + CREATE TABLE test (k int PRIMARY KEY, j int); + INSERT INTO test VALUES (1, 1000000); + INSERT INTO test SELECT g, g FROM generate_series(100, 2000) g; + CREATE INDEX test_j ON test(j); +} + +teardown +{ + DROP TABLE test; +} + +session s1 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; + SET enable_seqscan = off; +} +step s1_1 { SELECT k, j FROM test WHERE k = 1; } +step s1_2a { INSERT INTO test VALUES (1, 2); } +step s1_2b { INSERT INTO test VALUES (1, 2) ON CONFLICT DO NOTHING; } +step s1_3 { SELECT k, j FROM test WHERE k = 1 ORDER BY j; } +step c1 { COMMIT; } + +session s2 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; + SET enable_seqscan = off; +} +step s2_1 { DELETE FROM test WHERE j = 1000000; } +step c2 { COMMIT; } + +permutation s1_1 s2_1 c2 s1_2a s1_3 c1 +permutation s1_1 s2_1 c2 s1_2b s1_3 c1 diff --git a/src/test/isolation/specs/read-write-unique-6.spec b/src/test/isolation/specs/read-write-unique-6.spec new file mode 100644 index 00000000000..ca132f60ec6 --- /dev/null +++ b/src/test/isolation/specs/read-write-unique-6.spec @@ -0,0 +1,37 @@ +# Reusing a key that another transaction has deleted and committed, this time +# under an exclusion constraint. + +setup +{ + CREATE TABLE test (k int, j int, EXCLUDE USING btree (k WITH =)); + INSERT INTO test VALUES (1, 1000000); + INSERT INTO test SELECT g, g FROM generate_series(100, 2000) g; + CREATE INDEX test_j ON test(j); +} + +teardown +{ + DROP TABLE test; +} + +session s1 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; + SET enable_seqscan = off; +} +step s1_1 { SELECT k, j FROM test WHERE k = 1; } +step s1_2 { INSERT INTO test VALUES (1, 2); } +step s1_3 { SELECT k, j FROM test WHERE k = 1 ORDER BY j; } +step c1 { COMMIT; } + +session s2 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; + SET enable_seqscan = off; +} +step s2_1 { DELETE FROM test WHERE j = 1000000; } +step c2 { COMMIT; } + +permutation s1_1 s2_1 c2 s1_2 s1_3 c1 diff --git a/src/test/isolation/specs/read-write-unique-7.spec b/src/test/isolation/specs/read-write-unique-7.spec new file mode 100644 index 00000000000..01092ae44c3 --- /dev/null +++ b/src/test/isolation/specs/read-write-unique-7.spec @@ -0,0 +1,37 @@ +# Reusing a key that another transaction has deleted and committed, this time +# under a deferrable unique constraint. + +setup +{ + CREATE TABLE test (k int, j int, UNIQUE (k) DEFERRABLE INITIALLY DEFERRED); + INSERT INTO test VALUES (1, 1000000); + INSERT INTO test SELECT g, g FROM generate_series(100, 2000) g; + CREATE INDEX test_j ON test(j); +} + +teardown +{ + DROP TABLE test; +} + +session s1 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; + SET enable_seqscan = off; +} +step s1_1 { SELECT k, j FROM test WHERE k = 1; } +step s1_2 { INSERT INTO test VALUES (1, 2); } +step s1_3 { SELECT k, j FROM test WHERE k = 1 ORDER BY j; } +step c1 { COMMIT; } + +session s2 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; + SET enable_seqscan = off; +} +step s2_1 { DELETE FROM test WHERE j = 1000000; } +step c2 { COMMIT; } + +permutation s1_1 s2_1 s1_2 c2 s1_3 c1 -- 2.43.0
