From ef52d151b6429101810729b7e7f2ae256b7b1755 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Thu, 23 Jul 2026 20:18:45 +0000 Subject: [PATCH] Take SIREAD lock on rows read by ON CONFLICT DO SELECT The arbiter index probe runs with a dirty snapshot and the conflicting row is fetched with SnapshotAny, so no predicate lock is ever taken on the row that DO SELECT hands back, and the SELECT path writes nothing that would trigger conflict-in detection. A serializable transaction could therefore read the existing row without SSI recording the read, letting a concurrent writer of that row go unnoticed and allowing write skew. Lock the tuple in ExecOnConflictSelect before the WHERE clause is evaluated, so rows filtered out by it are covered as well. --- src/backend/executor/nodeModifyTable.c | 22 +++++++++ .../expected/insert-conflict-do-select-2.out | 40 +++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/insert-conflict-do-select-2.spec | 45 +++++++++++++++++++ 4 files changed, 108 insertions(+) create mode 100644 src/test/isolation/expected/insert-conflict-do-select-2.out create mode 100644 src/test/isolation/specs/insert-conflict-do-select-2.spec diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index ca954729f1e..558189fb09a 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -70,6 +70,7 @@ #include "rewrite/rewriteHandler.h" #include "rewrite/rewriteManip.h" #include "storage/lmgr.h" +#include "storage/predicate.h" #include "utils/builtins.h" #include "utils/datum.h" #include "utils/injection_point.h" @@ -3337,6 +3338,27 @@ ExecOnConflictSelect(ModifyTableContext *context, return false; } + /* + * At SERIALIZABLE, record an SIREAD lock on the tuple. Returning the + * existing row (or filtering it out with the WHERE clause) is a read for + * SSI purposes, but neither the arbiter index probe (dirty snapshot) nor + * the fetch above (SnapshotAny) takes predicate locks, and the SELECT + * path writes nothing that would trigger conflict-in detection. + */ + if (IsolationIsSerializable()) + { + Datum xminDatum; + TransactionId xmin; + bool isnull; + + xminDatum = slot_getsysattr(existing, MinTransactionIdAttributeNumber, &isnull); + Assert(!isnull); + xmin = DatumGetTransactionId(xminDatum); + + PredicateLockTID(relation, conflictTid, context->estate->es_snapshot, + xmin); + } + /* * Verify that the tuple is visible to our MVCC snapshot if the current * isolation level mandates that. See comments in ExecOnConflictUpdate(). diff --git a/src/test/isolation/expected/insert-conflict-do-select-2.out b/src/test/isolation/expected/insert-conflict-do-select-2.out new file mode 100644 index 00000000000..b0af050897f --- /dev/null +++ b/src/test/isolation/expected/insert-conflict-do-select-2.out @@ -0,0 +1,40 @@ +Parsed test spec with 2 sessions + +starting permutation: select1 count2 update2 insert1 c1 c2 +step select1: INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; +val +--- + 0 +(1 row) + +step count2: SELECT count(*) FROM doselect_b; +count +----- + 0 +(1 row) + +step update2: UPDATE doselect_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO doselect_b VALUES (1, 10); +step c1: COMMIT; +step c2: COMMIT; +ERROR: could not serialize access due to read/write dependencies among transactions + +starting permutation: select1_update count2 update2 insert1 c1 c2 +step select1_update: INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR UPDATE RETURNING val; +val +--- + 0 +(1 row) + +step count2: SELECT count(*) FROM doselect_b; +count +----- + 0 +(1 row) + +step update2: UPDATE doselect_a SET val = 1 WHERE key = 1; +step insert1: INSERT INTO doselect_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; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index 6469aafa2e1..6a0539fbfa0 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-do-select-2 test: merge-insert-update test: merge-delete test: merge-update diff --git a/src/test/isolation/specs/insert-conflict-do-select-2.spec b/src/test/isolation/specs/insert-conflict-do-select-2.spec new file mode 100644 index 00000000000..0c53a078d9e --- /dev/null +++ b/src/test/isolation/specs/insert-conflict-do-select-2.spec @@ -0,0 +1,45 @@ +# INSERT...ON CONFLICT DO SELECT test at SERIALIZABLE +# +# Returning the existing row is a read for SSI purposes, so 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 doselect_a (key int PRIMARY KEY, val int); + CREATE TABLE doselect_b (key int PRIMARY KEY, val int); + INSERT INTO doselect_a VALUES (1, 0); +} + +teardown +{ + DROP TABLE doselect_a, doselect_b; +} + +session s1 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; +} +step select1 { INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; } +step select1_update { INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR UPDATE RETURNING val; } +step insert1 { INSERT INTO doselect_b VALUES (1, 10); } +step c1 { COMMIT; } + +session s2 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; +} +step count2 { SELECT count(*) FROM doselect_b; } +step update2 { UPDATE doselect_a SET val = 1 WHERE key = 1; } +step c2 { COMMIT; } + +# Write skew with DO SELECT: s2 must fail to commit +permutation select1 count2 update2 insert1 c1 c2 + +# Write skew with DO SELECT FOR UPDATE: update2 blocks on the tuple lock +# and must fail once s1 commits +permutation select1_update count2 update2 insert1 c1 c2 -- 2.55.0