From 704f7367bff7384eedbf65b5dbc3b9c8930fd9d5 Mon Sep 17 00:00:00 2001 From: Zsolt Parragi Date: Mon, 7 Sep 2026 17:12:42 +0000 Subject: [PATCH v4] 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 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. 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 --- src/backend/executor/execIndexing.c | 19 +++ .../expected/insert-conflict-serializable.out | 116 ++++++++++++++++++ src/test/isolation/isolation_schedule | 1 + .../specs/insert-conflict-serializable.spec | 71 +++++++++++ src/test/modules/injection_points/Makefile | 3 +- .../expected/on_conflict_probe_window.out | 113 +++++++++++++++++ src/test/modules/injection_points/meson.build | 1 + .../specs/on_conflict_probe_window.spec | 57 +++++++++ 8 files changed, 380 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 eb383812901..1e3a86aca1b 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -908,7 +908,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", NULL); + (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..bc3ca7618e3 --- /dev/null +++ b/src/test/isolation/expected/insert-conflict-serializable.out @@ -0,0 +1,116 @@ +Parsed test spec with 2 sessions + +starting permutation: nothing1 count2 delete2 insert1 c1 c2 +step 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: nothing1 count2 update2 insert1 c1 c2 +step 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: update1_where count2 update2 insert1 c1 c2 +step 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: select1 count2 update2 insert1 c1 c2 +step 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: select1_keyshare count2 update2 insert1 c1 c2 +step 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: select1_where count2 update2 insert1 c1 c2 +step 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 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 select1: INSERT INTO ioc_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 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 1fcf4e63238..1916f93683a 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..febec86b0af --- /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 nothing1 { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; } +step update1_where { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO UPDATE SET val = 99 WHERE ioc_a.val > 100; } +step select1 { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; } +step select1_keyshare { INSERT INTO ioc_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE RETURNING val; } +step 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 nothing1 count2 delete2 insert1 c1 c2 +permutation 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 update1_where count2 update2 insert1 c1 c2 + +# DO SELECT returns the existing row: s2 must fail +permutation 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 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 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 select1 c2 insert1 c1 diff --git a/src/test/modules/injection_points/Makefile b/src/test/modules/injection_points/Makefile index 3b136adf126..524a15b531b 100644 --- a/src/test/modules/injection_points/Makefile +++ b/src/test/modules/injection_points/Makefile @@ -22,7 +22,8 @@ ISOLATION = basic \ ri_fastpath_reindex \ syscache-update-pruned \ wait_cleanup \ - heap_lock_update + heap_lock_update \ + on_conflict_probe_window # some isolation tests require wal_level=replica ISOLATION_OPTS = --temp-config $(top_srcdir)/src/test/modules/injection_points/extra.conf 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..a1498d86814 --- /dev/null +++ b/src/test/modules/injection_points/expected/on_conflict_probe_window.out @@ -0,0 +1,113 @@ +Parsed test spec with 2 sessions + +starting permutation: nothing1 count2 update2 c2 wake2 insert1 c1 +injection_points_attach +----------------------- + +(1 row) + +step 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 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; + +starting permutation: select1 count2 update2 c2 wake2 insert1 c1 +injection_points_attach +----------------------- + +(1 row) + +step select1: INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; +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 select1: <... completed> +val +--- + 0 +(1 row) + +step insert1: INSERT INTO probe_b VALUES (1, 10); +ERROR: could not serialize access due to read/write dependencies among transactions +step c1: COMMIT; + +starting permutation: select1_keyshare count2 update2 c2 wake2 insert1 c1 +injection_points_attach +----------------------- + +(1 row) + +step select1_keyshare: INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE RETURNING val; +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 select1_keyshare: <... completed> +val +--- + 0 +(1 row) + +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 aff516b901a..3ce79c4d268 100644 --- a/src/test/modules/injection_points/meson.build +++ b/src/test/modules/injection_points/meson.build @@ -54,6 +54,7 @@ tests += { 'syscache-update-pruned', 'wait_cleanup', 'heap_lock_update', + 'on_conflict_probe_window', ], 'runningcheck': false, # see syscache-update-pruned # Some tests wait for all snapshots, so avoid parallel execution 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..5d8cd3dc7f2 --- /dev/null +++ b/src/test/modules/injection_points/specs/on_conflict_probe_window.spec @@ -0,0 +1,57 @@ +# 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. +# DO SELECT without a lock and with FOR KEY SHARE (which does not conflict +# with the non-key update) rely on SSI alone to notice this. + +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 nothing1 { INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO NOTHING; } +step select1 { INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO SELECT RETURNING val; } +step select1_keyshare { INSERT INTO probe_a VALUES (1, 99) ON CONFLICT (key) DO SELECT FOR KEY SHARE RETURNING val; } +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 nothing1 count2 update2 c2 wake2 insert1 c1 +permutation select1 count2 update2 c2 wake2 insert1 c1 +permutation select1_keyshare count2 update2 c2 wake2 insert1 c1 -- 2.43.0