From 49845206c850075d91e31e21fdb475b3b31bffa1 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Mon, 7 Sep 2026 17:12:42 +0000 Subject: [PATCH v5] Fix missing SIREAD lock on the row found by ON CONFLICT. INSERT ... ON CONFLICT decides what to do based on the conflicting row found by the arbiter index probe, but SSI never saw that read: the probe runs with a dirty snapshot, which predicate locking ignores, and the later fetch of the row uses SnapshotAny. When the statement then writes nothing, as with DO NOTHING, DO UPDATE with a WHERE clause rejecting the row, or DO SELECT, nothing records the read at all. A concurrent writer of that row went unnoticed and write skew could commit at SERIALIZABLE, even though the same schedule with a plain SELECT of the row fails with a serialization error. To fix, introduce a new special snapshot, SnapshotDirtySerializable, which has the same tuple visibility rules as a regular dirty snapshot, but which causes predicate locks to be taken, like an MVCC snapshot. This special snapshot is used when doing the arbiter index probe for INSERT ... ON CONFLICT, and so covers every conflict action, including rows that the WHERE clause of DO UPDATE or DO SELECT then rejects. The DO NOTHING and DO UPDATE cases have been broken since ON CONFLICT was added in 9.5; DO SELECT is new in v19. Backpatch to all supported branches. Author: Zsolt Parragi Author: Andrey Borodin Author: Dean Rasheed Reported-by: Andrey Borodin Reported-by: Zsolt Parragi Discussion: https://postgr.es/m/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru Discussion: https://postgr.es/m/CAN4CZFM1GkHJkpMeo4G5rxtacVsfeKCJYiik9E9AKX1E9VYQ1w@mail.gmail.com Backpatch-through: 14 --- src/backend/executor/execIndexing.c | 31 +++-- src/backend/storage/lmgr/predicate.c | 13 +- src/backend/utils/time/snapmgr.c | 1 + src/include/utils/snapmgr.h | 2 + .../expected/insert-conflict-serializable.out | 116 ++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/insert-conflict-serializable.spec | 71 +++++++++++ 7 files changed, 224 insertions(+), 11 deletions(-) create mode 100644 src/test/isolation/expected/insert-conflict-serializable.out create mode 100644 src/test/isolation/specs/insert-conflict-serializable.spec diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index eb383812901..68ab117cbb4 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -718,6 +718,7 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, IndexScanDesc index_scan; ScanKeyData scankeys[INDEX_MAX_KEYS]; SnapshotData DirtySnapshot; + Snapshot snapshot; int i; bool conflict; bool found_self; @@ -787,9 +788,21 @@ check_exclusion_or_unique_constraint(Relation heap, Relation index, /* * Search the tuples that are in the index for any violations, including - * tuples that aren't visible yet. + * tuples that aren't visible yet. If we're only reporting potential or + * actual violations (for example, an arbiter index probe for INSERT ... + * ON CONFLICT), we use SnapshotDirtySerializable, because the outcome of + * the command depends on the probe result, and so the probe needs to + * count as a read for SSI purposes. Otherwise, we'll error out if a + * violation occurs, so we can just use a regular dirty snapshot, which + * does no predicate locking. */ - InitDirtySnapshot(DirtySnapshot); + if (violationOK) + snapshot = SnapshotDirtySerializable; + else + { + InitDirtySnapshot(DirtySnapshot); + snapshot = &DirtySnapshot; + } for (i = 0; i < indnkeyatts; i++) { @@ -824,7 +837,7 @@ retry: conflict = false; found_self = false; index_scan = index_beginscan(heap, index, - &DirtySnapshot, NULL, indnkeyatts, 0, + snapshot, NULL, indnkeyatts, 0, SO_NONE); index_rescan(index_scan, scankeys, indnkeyatts, NULL, 0); @@ -879,21 +892,21 @@ retry: * happen often enough to be worth trying harder, and anyway we don't * want to hold any index internal locks while waiting. */ - xwait = TransactionIdIsValid(DirtySnapshot.xmin) ? - DirtySnapshot.xmin : DirtySnapshot.xmax; + xwait = TransactionIdIsValid(snapshot->xmin) ? + snapshot->xmin : snapshot->xmax; if (TransactionIdIsValid(xwait) && (waitMode == CEOUC_WAIT || (waitMode == CEOUC_LIVELOCK_PREVENTING_WAIT && - DirtySnapshot.speculativeToken && + snapshot->speculativeToken && TransactionIdPrecedes(GetCurrentTransactionId(), xwait)))) { reason_wait = indexInfo->ii_ExclusionOps ? XLTW_RecheckExclusionConstr : XLTW_InsertIndex; index_endscan(index_scan); - if (DirtySnapshot.speculativeToken) - SpeculativeInsertionWait(DirtySnapshot.xmin, - DirtySnapshot.speculativeToken); + if (snapshot->speculativeToken) + SpeculativeInsertionWait(snapshot->xmin, + snapshot->speculativeToken); else XactLockTableWait(xwait, heap, &existing_slot->tts_tid, reason_wait); diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c index 0ae85b7d5b4..27e74ad9e14 100644 --- a/src/backend/storage/lmgr/predicate.c +++ b/src/backend/storage/lmgr/predicate.c @@ -534,13 +534,22 @@ SerializationNeededForRead(Relation relation, Snapshot snapshot) return false; /* - * Don't acquire locks or conflict when scanning with a special snapshot. + * Don't acquire locks or conflict when scanning with a special snapshot, + * other than SnapshotDirtySerializable, which is a special case. + * * This excludes things like CLUSTER and REINDEX. They use the wholesale * functions TransferPredicateLocksToHeapRelation() and * CheckTableForSerializableConflictIn() to participate in serialization, * but the scans involved don't need serialization. + * + * The special snapshot SnapshotDirtySerializable is a dirty snapshot, + * used by check_exclusion_or_unique_constraint() to test for potential or + * actual constraint violations (for example, when probing the arbiter + * index for INSERT ... ON CONFLICT). Since the outcome of such commands + * is decided by the probe result, it must count as a read for + * serialization purposes. */ - if (!IsMVCCSnapshot(snapshot)) + if (!IsMVCCSnapshot(snapshot) && snapshot != SnapshotDirtySerializable) return false; /* diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c index bbb6cb87203..2d257dd7d08 100644 --- a/src/backend/utils/time/snapmgr.c +++ b/src/backend/utils/time/snapmgr.c @@ -144,6 +144,7 @@ static SnapshotData CatalogSnapshotData = {SNAPSHOT_MVCC}; SnapshotData SnapshotSelfData = {SNAPSHOT_SELF}; SnapshotData SnapshotAnyData = {SNAPSHOT_ANY}; SnapshotData SnapshotToastData = {SNAPSHOT_TOAST}; +SnapshotData SnapshotDirtySerializableData = {SNAPSHOT_DIRTY}; /* Pointers to valid snapshots */ static Snapshot CurrentSnapshot = NULL; diff --git a/src/include/utils/snapmgr.h b/src/include/utils/snapmgr.h index 1c550096393..a385c242dec 100644 --- a/src/include/utils/snapmgr.h +++ b/src/include/utils/snapmgr.h @@ -28,9 +28,11 @@ extern PGDLLIMPORT TransactionId RecentXmin; extern PGDLLIMPORT SnapshotData SnapshotSelfData; extern PGDLLIMPORT SnapshotData SnapshotAnyData; extern PGDLLIMPORT SnapshotData SnapshotToastData; +extern PGDLLIMPORT SnapshotData SnapshotDirtySerializableData; #define SnapshotSelf (&SnapshotSelfData) #define SnapshotAny (&SnapshotAnyData) +#define SnapshotDirtySerializable (&SnapshotDirtySerializableData) /* Use get_toast_snapshot() for the TOAST snapshot */ diff --git a/src/test/isolation/expected/insert-conflict-serializable.out b/src/test/isolation/expected/insert-conflict-serializable.out new file mode 100644 index 00000000000..3ce77f94f1a --- /dev/null +++ b/src/test/isolation/expected/insert-conflict-serializable.out @@ -0,0 +1,116 @@ +Parsed test spec with 2 sessions + +starting permutation: ioc_nothing1 count2 delete2 insert1 c1 c2 +step ioc_nothing1: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step delete2: DELETE FROM ioc_a WHERE key = 1; +step insert1: INSERT INTO ioc_b VALUES (1, 10); +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions + +starting permutation: ioc_nothing1 count2 update2 insert1 c1 c2 +step ioc_nothing1: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step update2: UPDATE ioc_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO ioc_b VALUES (1, 10); +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions + +starting permutation: ioc_update1_where count2 update2 insert1 c1 c2 +step ioc_update1_where: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO UPDATE SET val = 99 WHERE ioc_a.val > 100; +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step update2: UPDATE ioc_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO ioc_b VALUES (1, 10); +step c1: COMMIT; +step update2: <... completed> +ERROR: could not serialize access due to read/write dependencies among transactions +step c2: COMMIT; + +starting permutation: ioc_select1 count2 update2 insert1 c1 c2 +step ioc_select1: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; +val +--- + 0 +(1 row) + +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step update2: UPDATE ioc_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO ioc_b VALUES (1, 10); +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions + +starting permutation: ioc_select1_keyshare count2 update2 insert1 c1 c2 +step ioc_select1_keyshare: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE RETURNING val; +val +--- + 0 +(1 row) + +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step update2: UPDATE ioc_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO ioc_b VALUES (1, 10); +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions + +starting permutation: ioc_select1_where count2 update2 insert1 c1 c2 +step ioc_select1_where: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT WHERE ioc_a.val > 100 RETURNING val; +val +--- +(0 rows) + +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step update2: UPDATE ioc_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO ioc_b VALUES (1, 10); +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions + +starting permutation: count2 update2 ioc_select1 c2 insert1 c1 +step count2: SELECT count(*) FROM ioc_b; +count +----- + 0 +(1 row) + +step update2: UPDATE ioc_a SET val = 1 WHERE key = 1; +step ioc_select1: INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; +step c2: COMMIT; +step ioc_select1: <... completed> +ERROR: could not serialize access due to concurrent update +step insert1: INSERT INTO ioc_b VALUES (1, 10); +ERROR: current transaction is aborted, commands ignored until end of transaction block +step c1: COMMIT; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index fc45d504d2b..198773f4159 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -58,6 +58,7 @@ test: insert-conflict-do-update-3 test: insert-conflict-do-update-4 test: insert-conflict-specconflict test: insert-conflict-do-select +test: insert-conflict-serializable test: merge-insert-update test: merge-delete test: merge-update diff --git a/src/test/isolation/specs/insert-conflict-serializable.spec b/src/test/isolation/specs/insert-conflict-serializable.spec new file mode 100644 index 00000000000..c177003279b --- /dev/null +++ b/src/test/isolation/specs/insert-conflict-serializable.spec @@ -0,0 +1,71 @@ +# INSERT ... ON CONFLICT at SERIALIZABLE +# +# The conflicting row decides the outcome of the statement, so it counts +# as a read for SSI purposes, whether or not the statement then writes +# anything: a concurrent transaction writing that row must create a +# rw-antidependency. These permutations build the classic write-skew +# cycle: s1 reads a and writes b, while s2 reads b and writes a. One of +# the two transactions must fail with a serialization error. + +setup +{ + CREATE TABLE ioc_a (key int PRIMARY KEY, val int); + CREATE TABLE ioc_b (key int PRIMARY KEY, val int); + INSERT INTO ioc_a VALUES (1, 0); +} + +teardown +{ + DROP TABLE ioc_a, ioc_b; +} + +session s1 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; +} +step ioc_nothing1 { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; } +step ioc_update1_where { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO UPDATE SET val = 99 WHERE ioc_a.val > 100; } +step ioc_select1 { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; } +step ioc_select1_keyshare { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE RETURNING val; } +step ioc_select1_where { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT WHERE ioc_a.val > 100 RETURNING val; } +step insert1 { INSERT INTO ioc_b VALUES (1, 10); } +step c1 { COMMIT; } + +session s2 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; +} +step count2 { SELECT count(*) FROM ioc_b; } +step update2 { UPDATE ioc_a SET val = 1 WHERE key = 1; } +step delete2 { DELETE FROM ioc_a WHERE key = 1; } +step c2 { COMMIT; } + +# DO NOTHING skips the insert because of the existing row, which s2 then +# deletes or updates: s2 must fail to commit +permutation ioc_nothing1 count2 delete2 insert1 c1 c2 +permutation ioc_nothing1 count2 update2 insert1 c1 c2 + +# DO UPDATE with a WHERE clause rejecting the existing row writes nothing, +# but the row still decided the outcome: s2 must fail +permutation ioc_update1_where count2 update2 insert1 c1 c2 + +# DO SELECT returns the existing row: s2 must fail +permutation ioc_select1 count2 update2 insert1 c1 c2 + +# DO SELECT FOR KEY SHARE: the non-key update does not conflict with the +# tuple lock, so update2 proceeds without blocking and only the SIREAD +# lock makes s2 fail +permutation ioc_select1_keyshare count2 update2 insert1 c1 c2 + +# DO SELECT with a WHERE clause rejecting the existing row: the row is not +# returned, but it decided the outcome and was examined by the WHERE +# clause, so it still counts as a read +permutation ioc_select1_where count2 update2 insert1 c1 c2 + +# If the update is already in flight when DO SELECT runs, the arbiter +# probe waits for it to commit, leaving a conflicting row that is not +# visible to the query snapshot. The row cannot be returned, so DO +# SELECT must fail instead +permutation count2 update2 ioc_select1 c2 insert1 c1 -- 2.51.0