From fe46617e1dd309dd04c5cc3ec0199e2c043d8e2b Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Thu, 23 Jul 2026 20:18:45 +0000 Subject: [PATCH v3] 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. To fix, read the tuple with the query snapshot in ExecOnConflictSelect, making the read visible to SSI. This happens before the tuple lock is taken, which can succeed without noticing a concurrent writer (FOR KEY SHARE does not conflict with a non-key update), and before the WHERE clause is evaluated, so rows filtered out by it are covered as well. --- src/backend/executor/nodeModifyTable.c | 26 ++++++- .../expected/insert-conflict-do-select-2.out | 73 +++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/insert-conflict-do-select-2.spec | 58 +++++++++++++++ 4 files changed, 157 insertions(+), 1 deletion(-) 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..d092eeea127 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -3293,6 +3293,7 @@ ExecOnConflictSelect(ModifyTableContext *context, ExprState *onConflictSelectWhere = resultRelInfo->ri_onConflict->oc_WhereClause; TupleTableSlot *existing = resultRelInfo->ri_onConflict->oc_Existing; LockClauseStrength lockStrength = resultRelInfo->ri_onConflict->oc_LockStrength; + bool fetched = false; /* * Parse analysis should have blocked ON CONFLICT for all system @@ -3301,10 +3302,33 @@ ExecOnConflictSelect(ModifyTableContext *context, */ Assert(!resultRelInfo->ri_needLockTagTuple); + /* + * At SERIALIZABLE, read the tuple with the query snapshot first: a read + * only becomes visible to SSI when it is made through an MVCC snapshot, + * and none of the other reads here qualifies -- the arbiter index probe + * uses a dirty snapshot, the fetch below SnapshotAny, and the tuple lock + * checks no snapshot at all -- while the SELECT path writes nothing that + * would make the conflict visible from the writer's side. This read + * must precede the tuple lock: the lock can succeed without noticing a + * concurrent writer, as FOR KEY SHARE does not conflict with a non-key + * update. + * + * A tuple invisible to the query snapshot was written either by this + * transaction, which needs no conflict detection against itself, or by + * a concurrent one, which ExecCheckTupleVisible below turns into a + * serialization failure. + */ + if (IsolationIsSerializable()) + fetched = table_tuple_fetch_row_version(relation, + conflictTid, + context->estate->es_snapshot, + existing); + /* Fetch/lock existing tuple, according to the requested lock strength */ if (lockStrength == LCS_NONE) { - if (!table_tuple_fetch_row_version(relation, + if (!fetched && + !table_tuple_fetch_row_version(relation, conflictTid, SnapshotAny, existing)) 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..4a5ce282949 --- /dev/null +++ b/src/test/isolation/expected/insert-conflict-do-select-2.out @@ -0,0 +1,73 @@ +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_keyshare count2 update2 insert1 c1 c2 +step select1_keyshare: INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE 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: count2 update2 select1 c2 insert1 c1 +step count2: SELECT count(*) FROM doselect_b; +count +----- + 0 +(1 row) + +step update2: UPDATE doselect_a SET val = 1 WHERE key = 1; +step select1: INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; +step c2: COMMIT; +step select1: <... completed> +ERROR: could not serialize access due to concurrent update +step insert1: INSERT INTO doselect_b VALUES (1, 10); +ERROR: current transaction is aborted, commands ignored until end of transaction block +step c1: COMMIT; + +starting permutation: select1_where count2 update2 insert1 c1 c2 +step select1_where: INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT WHERE doselect_a.val > 100 RETURNING val; +val +--- +(0 rows) + +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 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..60b9a5a468d --- /dev/null +++ b/src/test/isolation/specs/insert-conflict-do-select-2.spec @@ -0,0 +1,58 @@ +# 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_keyshare { INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE RETURNING val; } +step select1_where { INSERT INTO doselect_a VALUES (1, 99) ON CONFLICT (key) DO SELECT WHERE doselect_a.val > 100 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 KEY SHARE: the non-key update does not +# conflict with the tuple lock, so update2 proceeds without blocking and +# only the SIREAD lock makes it fail +permutation select1_keyshare 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 select1 c2 insert1 c1 + +# Write skew with a DO SELECT WHERE clause that rejects the existing row: +# the row is not returned, but it decided the outcome of the INSERT and +# was examined by the WHERE clause, so it still counts as a read +permutation select1_where count2 update2 insert1 c1 c2 -- 2.55.0