From 5b4ebfbfaec322dd7b2d2ee25075ae9efe9bee0a Mon Sep 17 00:00:00 2001 From: Dean Rasheed Date: Tue, 15 Sep 2026 11:47:08 +0100 Subject: [PATCH] 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, read the conflicting tuple again with the query snapshot, right where the probe finds it. The table AM takes the SIREAD lock and checks for a concurrent writer of the tuple as part of that read, both under the buffer lock, so a writer either sees the lock or is seen. A predicate lock by itself acquired separately after the probe could not offer that: a writer passing its conflict check in between would be missed. Doing this in the probe 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 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 | 20 ++++++++ .../expected/insert-conflict-serializable.out | 44 ++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/insert-conflict-serializable.spec | 49 ++++++++++++++++++ src/test/modules/injection_points/Makefile | 3 +- .../expected/on_conflict_probe_window.out | 35 +++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/on_conflict_probe_window.spec | 51 +++++++++++++++++++ 8 files changed, 203 insertions(+), 1 deletion(-) create mode 100644 src/test/isolation/expected/insert-conflict-serializable.out create mode 100644 src/test/isolation/specs/insert-conflict-serializable.spec create mode 100644 src/test/modules/injection_points/expected/on_conflict_probe_window.out create mode 100644 src/test/modules/injection_points/specs/on_conflict_probe_window.spec diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index dded5e79374..1122d88bdd5 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 "utils/injection_point.h" #include "utils/snapmgr.h" /* waitMode argument to check_exclusion_or_unique_constraint() */ @@ -857,7 +858,26 @@ retry: { conflict = true; if (conflictTid) + { *conflictTid = existing_slot->tts_tid; + + /* + * The conflicting tuple decides the outcome of INSERT ... ON + * CONFLICT, so for SSI purposes it has been read, even when + * nothing gets written afterwards. The dirty snapshot used + * by the scan is not an MVCC snapshot, so SSI ignored that + * read. Read the tuple again with the query snapshot to + * record it. The result is of no interest here, the caller + * checks visibility itself. + */ + if (IsolationIsSerializable()) + { + INJECTION_POINT("check-exclusion-or-unique-constraint-conflict"); + (void) table_tuple_fetch_row_version(heap, conflictTid, + estate->es_snapshot, + existing_slot); + } + } break; } 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..6c4abe671f5 --- /dev/null +++ b/src/test/isolation/expected/insert-conflict-serializable.out @@ -0,0 +1,44 @@ +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; diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule index db479c35204..6b19c0c9ec3 100644 --- a/src/test/isolation/isolation_schedule +++ b/src/test/isolation/isolation_schedule @@ -51,6 +51,7 @@ test: insert-conflict-do-update-2 test: insert-conflict-do-update-3 test: insert-conflict-do-update-4 test: insert-conflict-specconflict +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..e4fb60b7ac9 --- /dev/null +++ b/src/test/isolation/specs/insert-conflict-serializable.spec @@ -0,0 +1,49 @@ +# 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 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 diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 42981bd0880..5b96110209a 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -16,7 +16,8 @@ ISOLATION = inplace \ reindex_concurrently_deferred \ syscache-update-pruned \ wait_cleanup \ - heap_lock_update + heap_lock_update \ + on_conflict_probe_window # The injection points are cluster-wide, so disable installcheck NO_INSTALLCHECK = 1 diff --git a/src/test/modules/injection_points/expected/on_conflict_probe_window.out b/src/test/modules/injection_points/expected/on_conflict_probe_window.out new file mode 100644 index 00000000000..d2b9a6b73dc --- /dev/null +++ b/src/test/modules/injection_points/expected/on_conflict_probe_window.out @@ -0,0 +1,35 @@ +Parsed test spec with 2 sessions + +starting permutation: ioc_nothing1 count2 update2 c2 wake2 insert1 c1 +injection_points_attach +----------------------- + +(1 row) + +step ioc_nothing1: INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; +step count2: SELECT count(*) FROM probe_b; +count +----- + 0 +(1 row) + +step update2: UPDATE probe_a SET val = 1 WHERE key = 1; +step c2: COMMIT; +step wake2: + SELECT injection_points_detach('check-exclusion-or-unique-constraint-conflict'); + SELECT injection_points_wakeup('check-exclusion-or-unique-constraint-conflict'); + +injection_points_detach +----------------------- + +(1 row) + +injection_points_wakeup +----------------------- + +(1 row) + +step ioc_nothing1: <... completed> +step insert1: INSERT INTO probe_b VALUES (1, 10); +ERROR: could not serialize access due to read/write dependencies among transactions +step c1: COMMIT; diff --git a/src/test/modules/injection_points/meson.build b/src/test/modules/injection_points/meson.build index 05f0d529e7f..425a465b3d3 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -47,6 +47,7 @@ tests += { 'syscache-update-pruned', 'wait_cleanup', 'heap_lock_update', + 'on_conflict_probe_window', ], 'runningcheck': false, # see syscache-update-pruned }, diff --git a/src/test/modules/injection_points/specs/on_conflict_probe_window.spec b/src/test/modules/injection_points/specs/on_conflict_probe_window.spec new file mode 100644 index 00000000000..91a2bf41d64 --- /dev/null +++ b/src/test/modules/injection_points/specs/on_conflict_probe_window.spec @@ -0,0 +1,51 @@ +# INSERT ... ON CONFLICT arbiter probe vs. a concurrent writer at SERIALIZABLE +# +# The arbiter probe reads the conflicting row to decide the statement's +# outcome, so the read must be visible to SSI atomically: any SIREAD lock +# taken after the probe leaves a window in which a concurrent writer of +# the row checks for conflicts without seeing it. The injection point +# holds the statement right after the probe picked the conflicting tuple, +# while s2 updates the row and commits. ON CONFLICT then decides based +# on a row version that s2 already replaced, completing the write-skew +# cycle: s1 reads a and writes b, s2 reads b and writes a. s1 must fail. + +setup +{ + CREATE EXTENSION injection_points; + CREATE TABLE probe_a (key int PRIMARY KEY, val int); + CREATE TABLE probe_b (key int PRIMARY KEY, val int); + INSERT INTO probe_a VALUES (1, 0); + -- attached globally, the point has to fire in session s1 + SELECT injection_points_attach('check-exclusion-or-unique-constraint-conflict', 'wait'); +} + +teardown +{ + DROP TABLE probe_a, probe_b; + DROP EXTENSION injection_points; +} + +session s1 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; +} +step ioc_nothing1 { INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; } +step insert1 { INSERT INTO probe_b VALUES (1, 10); } +step c1 { COMMIT; } + +session s2 +setup +{ + BEGIN ISOLATION LEVEL SERIALIZABLE; +} +step count2 { SELECT count(*) FROM probe_b; } +step update2 { UPDATE probe_a SET val = 1 WHERE key = 1; } +step c2 { COMMIT; } +step wake2 +{ + SELECT injection_points_detach('check-exclusion-or-unique-constraint-conflict'); + SELECT injection_points_wakeup('check-exclusion-or-unique-constraint-conflict'); +} + +permutation ioc_nothing1 count2 update2 c2 wake2 insert1 c1