Hi, > Actually I think we should do just the opposite - focus on the patch > that can be backported and discuss possible improvements for the > `master` branch later.
> 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. I think I found a fix that doesn't break ABI or the existing behaviour. It introduces a per-backend global variable which is not pretty, but it works and we can always refactor the code in the `master` branch after backpatching. Please see PoC v2. Any feedback is most welcome. -- Best regards, Aleksander Alekseev
From 422b851d34e6f27b6f911cc1f45074ee2a23c241 Mon Sep 17 00:00:00 2001 From: Aleksander Alekseev <[email protected]> Date: Thu, 8 Oct 2026 15:45:37 +0300 Subject: [PATCH v2] PoC bugfix by a.alekseev v2 https://postgr.es/m/CA%2BCOZaBOiiRPmEfX00oE%3DN6HBSZVe0Y-y-ZqqaXq8BAAj1gu%2BQ%40mail.gmail.com --- src/backend/access/nbtree/nbtinsert.c | 59 ++++++++------- src/backend/executor/execIndexing.c | 51 +++++++++++++ src/backend/storage/lmgr/predicate.c | 73 +++++++++++++++++++ src/include/storage/predicate.h | 4 + .../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, 346 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..8c6449bac62 100644 --- a/src/backend/access/nbtree/nbtinsert.c +++ b/src/backend/access/nbtree/nbtinsert.c @@ -674,36 +674,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..db1e919b74a 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,56 @@ 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 to see whether any of them is still ours. + */ + if (!conflict && ActiveSnapshotSet() && + CheckForSerializableConflictOutNeeded(heap, GetActiveSnapshot())) + { + Datum existing_values[INDEX_MAX_KEYS]; + bool existing_isnull[INDEX_MAX_KEYS]; + uint64 conflicts; + + index_scan = index_beginscan(heap, index, false, + GetActiveSnapshot(), NULL, indnkeyatts, 0, + SO_NONE); + index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0); + + for (;;) + { + conflicts = GetSerializableConflictOutCount(); + + if (!table_index_getnext_slot(index_scan, ForwardScanDirection, + existing_slot)) + break; + + if (ItemPointerIsValid(tupleid) && + ItemPointerEquals(tupleid, &existing_slot->tts_tid)) + continue; + + /* + * A row we can still see is only in our way if a concurrent + * serializable transaction is the one that deleted it. + */ + if (GetSerializableConflictOutCount() == conflicts) + 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; + + ReportSerializableKeyReuse(); + } + + 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..cf9bd740c4b 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" @@ -434,6 +435,12 @@ static HTAB *LocalPredicateLockHash = NULL; static SERIALIZABLEXACT *MySerializableXact = InvalidSerializableXact; static bool MyXactDidWrite = false; +/* + * Counts reads which conflicted out to a concurrent serializable transaction. + * Never reset; see GetSerializableConflictOutCount(). + */ +static uint64 SerializableConflictOutCount = 0; + /* * The SXACT_FLAG_RO_UNSAFE optimization might lead us to release * MySerializableXact early. If that happens in a parallel query, the leader @@ -4009,6 +4016,7 @@ CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot s errhint("The transaction might succeed if retried."))); MySerializableXact->flags |= SXACT_FLAG_SUMMARY_CONFLICT_OUT; + SerializableConflictOutCount++; } /* It's not serializable or otherwise not important. */ @@ -4075,6 +4083,7 @@ CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot s if (RWConflictExists(MySerializableXact, sxact)) { /* We don't want duplicate conflict records in the list. */ + SerializableConflictOutCount++; LWLockRelease(SerializableXactHashLock); return; } @@ -4083,6 +4092,7 @@ CheckForSerializableConflictOut(Relation relation, TransactionId xid, Snapshot s * Flag the conflict. But first, if this conflict creates a dangerous * structure, ereport an error. */ + SerializableConflictOutCount++; FlagRWConflict(MySerializableXact, sxact); LWLockRelease(SerializableXactHashLock); } @@ -4419,6 +4429,69 @@ CheckTableForSerializableConflictIn(Relation relation) LWLockRelease(SerializablePredicateListLock); } +/* + * Report a key reuse the caller has established. + */ +void +ReportSerializableKeyReuse(void) +{ + 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."))); +} + +/* + * Report how many reads have conflicted out to a concurrent serializable + * transaction. Sample this before and after a read to learn whether that read + * did so. The counter is never reset, so compare samples for equality. + */ +uint64 +GetSerializableConflictOutCount(void) +{ + return SerializableConflictOutCount; +} + +/* + * 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) +{ + ItemPointerData htid; + Snapshot snapshot; + uint64 conflicts; + + if (!ActiveSnapshotSet()) + return; + snapshot = GetActiveSnapshot(); + + if (!CheckForSerializableConflictOutNeeded(relation, snapshot)) + return; + + htid = *tid; + conflicts = SerializableConflictOutCount; + + /* + * The fetch reports a conflict out to whoever deleted the tuple we can + * still see. No conflict means the deleter was ourselves or was not + * serializable, and the key is ours to reuse. + */ + if (!table_fetch_tid(relation, &htid, snapshot, NULL)) + return; + + if (SerializableConflictOutCount == conflicts) + return; + + ReportSerializableKeyReuse(); +} + /* * Flag a rw-dependency between two serializable transactions. diff --git a/src/include/storage/predicate.h b/src/include/storage/predicate.h index 443bffb58fd..debd21093eb 100644 --- a/src/include/storage/predicate.h +++ b/src/include/storage/predicate.h @@ -66,6 +66,10 @@ 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); +extern void ReportSerializableKeyReuse(void); +extern uint64 GetSerializableConflictOutCount(void); /* 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 bf9a2037c70..3e29d5cdba2 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
