public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andrey Borodin <[email protected]>
To: Kyle Kingsbury <[email protected]>
Cc: PostgreSQL mailing lists <[email protected]>
Subject: Re: Possible G2-item at SERIALIZABLE
Date: Mon, 1 Jun 2026 15:11:46 +0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
<[email protected]>
<[email protected]>
> On 1 Jun 2026, at 11:18, Andrey Borodin <[email protected]> wrote:
>
> There's a separate case
Two more SSI false-negatives in the same area, found by Nik's machines
and Mark-bot while triaging original report.
INSERT ... ON CONFLICT reads the conflicting ("arbiter") row to decide
what to do, but doesn't take an SIREAD predicate lock on it. So when
the statement ultimately writes no tuple, that read leaves no trace for
SSI and a concurrent modification of the same row can produce a
non-serializable all-commit result:
* ON CONFLICT DO UPDATE ... WHERE <false> — the no-op update branch; the
conflict row is observed but not updated.
* ON CONFLICT DO NOTHING with a concurrent DELETE of the conflict row.
Replacing the ON CONFLICT with a plain SELECT of the same row aborts
correctly, which is what convinced me the schedules are genuinely
non-serializable and the gap is in the ON CONFLICT probe path.
Both come from the same place - check_exclusion_or_unique_constraint() in
execIndexing.c finds the arbiter tuple but never calls PredicateLockTID().
Adding that lock there fixes both.
There's also one more false negative, but it is in -DTEST_SUMMARIZE_SERIAL
and IMO worth working only when we deal with what we have in production
cases.
Best regards, Andrey Borodin.
Attachments:
[application/octet-stream] 0001-Add-isolation-tests-for-ON-CONFLICT-no-op-SSI-false-.patch (10.3K, 2-0001-Add-isolation-tests-for-ON-CONFLICT-no-op-SSI-false-.patch)
download | inline diff:
From fd43ff3f82d4edb5df6ca59e13aeeaa12c296da9 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Mon, 1 Jun 2026 12:31:34 +0500
Subject: [PATCH 1/2] Add isolation tests for ON CONFLICT no-op SSI false
negatives
INSERT ... ON CONFLICT DO NOTHING, and DO UPDATE with an unsatisfied
WHERE, read the conflicting row but take no SIREAD lock, so a concurrent
UPDATE/DELETE of it can escape write-skew detection under SERIALIZABLE.
Each spec contrasts a plain-SELECT control with the ON CONFLICT variant.
The expected output encodes the serializable result (one transaction
cancelled), so these tests fail until the accompanying fix is applied.
---
.../serializable-onconflict-do-nothing.out | 68 ++++++++++++++++++
.../serializable-onconflict-noop-update.out | 69 +++++++++++++++++++
src/test/isolation/isolation_schedule | 3 +
.../serializable-onconflict-do-nothing.spec | 42 +++++++++++
.../serializable-onconflict-noop-update.spec | 42 +++++++++++
5 files changed, 224 insertions(+)
create mode 100644 src/test/isolation/expected/serializable-onconflict-do-nothing.out
create mode 100644 src/test/isolation/expected/serializable-onconflict-noop-update.out
create mode 100644 src/test/isolation/specs/serializable-onconflict-do-nothing.spec
create mode 100644 src/test/isolation/specs/serializable-onconflict-noop-update.spec
diff --git a/src/test/isolation/expected/serializable-onconflict-do-nothing.out b/src/test/isolation/expected/serializable-onconflict-do-nothing.out
new file mode 100644
index 00000000000..45ef582465d
--- /dev/null
+++ b/src/test/isolation/expected/serializable-onconflict-do-nothing.out
@@ -0,0 +1,68 @@
+Parsed test spec with 3 sessions
+
+starting permutation: reset s1read s2r2 s1w2 s2w1 s1c s2c ck
+step reset: DELETE FROM noc; INSERT INTO noc VALUES (1, 0), (2, 0);
+step s1read: SELECT v FROM noc WHERE k = 1;
+v
+-
+0
+(1 row)
+
+step s2r2: SELECT v FROM noc WHERE k = 2;
+v
+-
+0
+(1 row)
+
+step s1w2: UPDATE noc SET v = 1 WHERE k = 2;
+step s2w1: DELETE FROM noc WHERE k = 1;
+step s1c: COMMIT;
+step s2c: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step ck: SELECT k, v FROM noc ORDER BY k;
+ SELECT CASE WHEN NOT EXISTS (SELECT 1 FROM noc WHERE k = 1)
+ AND (SELECT v FROM noc WHERE k = 2) = 1
+ THEN 'bad_both_committed'
+ ELSE 'ok_aborted_or_serial' END AS invariant;
+k|v
+-+-
+1|0
+2|1
+(2 rows)
+
+invariant
+--------------------
+ok_aborted_or_serial
+(1 row)
+
+
+starting permutation: reset s1noop s2r2 s1w2 s2w1 s1c s2c ck
+step reset: DELETE FROM noc; INSERT INTO noc VALUES (1, 0), (2, 0);
+step s1noop: INSERT INTO noc(k, v) VALUES (1, 99) ON CONFLICT (k) DO NOTHING;
+step s2r2: SELECT v FROM noc WHERE k = 2;
+v
+-
+0
+(1 row)
+
+step s1w2: UPDATE noc SET v = 1 WHERE k = 2;
+step s2w1: DELETE FROM noc WHERE k = 1;
+step s1c: COMMIT;
+step s2c: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step ck: SELECT k, v FROM noc ORDER BY k;
+ SELECT CASE WHEN NOT EXISTS (SELECT 1 FROM noc WHERE k = 1)
+ AND (SELECT v FROM noc WHERE k = 2) = 1
+ THEN 'bad_both_committed'
+ ELSE 'ok_aborted_or_serial' END AS invariant;
+k|v
+-+-
+1|0
+2|1
+(2 rows)
+
+invariant
+--------------------
+ok_aborted_or_serial
+(1 row)
+
diff --git a/src/test/isolation/expected/serializable-onconflict-noop-update.out b/src/test/isolation/expected/serializable-onconflict-noop-update.out
new file mode 100644
index 00000000000..af8ce068cc3
--- /dev/null
+++ b/src/test/isolation/expected/serializable-onconflict-noop-update.out
@@ -0,0 +1,69 @@
+Parsed test spec with 3 sessions
+
+starting permutation: reset s1read s2r2 s1w2 s2w1 s1c s2c ck
+step reset: DELETE FROM noc; INSERT INTO noc VALUES (1, 0), (2, 0);
+step s1read: SELECT v FROM noc WHERE k = 1;
+v
+-
+0
+(1 row)
+
+step s2r2: SELECT v FROM noc WHERE k = 2;
+v
+-
+0
+(1 row)
+
+step s1w2: UPDATE noc SET v = 1 WHERE k = 2;
+step s2w1: UPDATE noc SET v = 42 WHERE k = 1;
+step s1c: COMMIT;
+step s2c: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+step ck: SELECT k, v FROM noc ORDER BY k;
+ SELECT CASE WHEN (SELECT v FROM noc WHERE k = 1) = 42
+ AND (SELECT v FROM noc WHERE k = 2) = 1
+ THEN 'bad_both_committed'
+ ELSE 'ok_aborted_or_serial' END AS invariant;
+k|v
+-+-
+1|0
+2|1
+(2 rows)
+
+invariant
+--------------------
+ok_aborted_or_serial
+(1 row)
+
+
+starting permutation: reset s1noop s2r2 s1w2 s2w1 s1c s2c ck
+step reset: DELETE FROM noc; INSERT INTO noc VALUES (1, 0), (2, 0);
+step s1noop: INSERT INTO noc(k, v) VALUES (1, 99) ON CONFLICT (k) DO UPDATE SET v = 100 WHERE noc.v = 42;
+step s2r2: SELECT v FROM noc WHERE k = 2;
+v
+-
+0
+(1 row)
+
+step s1w2: UPDATE noc SET v = 1 WHERE k = 2;
+step s2w1: UPDATE noc SET v = 42 WHERE k = 1; <waiting ...>
+step s1c: COMMIT;
+step s2w1: <... completed>
+ERROR: could not serialize access due to read/write dependencies among transactions
+step s2c: COMMIT;
+step ck: SELECT k, v FROM noc ORDER BY k;
+ SELECT CASE WHEN (SELECT v FROM noc WHERE k = 1) = 42
+ AND (SELECT v FROM noc WHERE k = 2) = 1
+ THEN 'bad_both_committed'
+ ELSE 'ok_aborted_or_serial' END AS invariant;
+k|v
+-+-
+1|0
+2|1
+(2 rows)
+
+invariant
+--------------------
+ok_aborted_or_serial
+(1 row)
+
diff --git a/src/test/isolation/isolation_schedule b/src/test/isolation/isolation_schedule
index 15c33fad4c5..313e8abba6f 100644
--- a/src/test/isolation/isolation_schedule
+++ b/src/test/isolation/isolation_schedule
@@ -127,3 +127,6 @@ test: matview-write-skew
test: lock-nowait
test: for-portion-of
test: ddl-dependency-locking
+
+test: serializable-onconflict-noop-update
+test: serializable-onconflict-do-nothing
diff --git a/src/test/isolation/specs/serializable-onconflict-do-nothing.spec b/src/test/isolation/specs/serializable-onconflict-do-nothing.spec
new file mode 100644
index 00000000000..da8cd10d3f5
--- /dev/null
+++ b/src/test/isolation/specs/serializable-onconflict-do-nothing.spec
@@ -0,0 +1,42 @@
+# Write-skew via INSERT ... ON CONFLICT DO NOTHING with a concurrent delete of
+# the conflicting row under SERIALIZABLE.
+#
+# s1 examines k=1 through the ON CONFLICT arbiter and writes k=2
+# s2 reads k=2 and deletes k=1
+#
+# This is a dangerous structure, so SSI must cancel one transaction.
+#
+# The question this exercises: the arbiter's conflict probe is a read that
+# decides the statement's outcome -- exactly like the plain SELECT in the
+# control permutation -- so it must participate in SSI and take an SIREAD lock.
+# Until that read was predicate-locked, only the control was cancelled while
+# the ON CONFLICT DO NOTHING variant committed a non-serializable result.
+
+setup { CREATE TABLE noc (k int PRIMARY KEY, v int NOT NULL); }
+teardown { DROP TABLE noc; }
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s1read { SELECT v FROM noc WHERE k = 1; }
+step s1noop { INSERT INTO noc(k, v) VALUES (1, 99) ON CONFLICT (k) DO NOTHING; }
+step s1w2 { UPDATE noc SET v = 1 WHERE k = 2; }
+step s1c { COMMIT; }
+
+session s2
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s2r2 { SELECT v FROM noc WHERE k = 2; }
+step s2w1 { DELETE FROM noc WHERE k = 1; }
+step s2c { COMMIT; }
+
+# Non-transactional helper: resets the rows before each permutation and reports
+# the final committed state plus a serializability invariant.
+session ctl
+step reset { DELETE FROM noc; INSERT INTO noc VALUES (1, 0), (2, 0); }
+step ck { SELECT k, v FROM noc ORDER BY k;
+ SELECT CASE WHEN NOT EXISTS (SELECT 1 FROM noc WHERE k = 1)
+ AND (SELECT v FROM noc WHERE k = 2) = 1
+ THEN 'bad_both_committed'
+ ELSE 'ok_aborted_or_serial' END AS invariant; }
+
+permutation reset s1read s2r2 s1w2 s2w1 s1c s2c ck
+permutation reset s1noop s2r2 s1w2 s2w1 s1c s2c ck
diff --git a/src/test/isolation/specs/serializable-onconflict-noop-update.spec b/src/test/isolation/specs/serializable-onconflict-noop-update.spec
new file mode 100644
index 00000000000..44a52f4953c
--- /dev/null
+++ b/src/test/isolation/specs/serializable-onconflict-noop-update.spec
@@ -0,0 +1,42 @@
+# Write-skew via INSERT ... ON CONFLICT DO UPDATE with an unsatisfied WHERE
+# (a no-op update) under SERIALIZABLE.
+#
+# s1 examines k=1 through the ON CONFLICT arbiter and writes k=2
+# s2 reads k=2 and writes k=1
+#
+# This is a dangerous structure, so SSI must cancel one transaction.
+#
+# The question this exercises: the arbiter's conflict probe is a read that
+# decides the statement's outcome -- exactly like the plain SELECT in the
+# control permutation -- so it must participate in SSI and take an SIREAD lock.
+# Until that read was predicate-locked, only the control was cancelled while
+# the ON CONFLICT no-op variant committed a non-serializable result.
+
+setup { CREATE TABLE noc (k int PRIMARY KEY, v int NOT NULL); }
+teardown { DROP TABLE noc; }
+
+session s1
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s1read { SELECT v FROM noc WHERE k = 1; }
+step s1noop { INSERT INTO noc(k, v) VALUES (1, 99) ON CONFLICT (k) DO UPDATE SET v = 100 WHERE noc.v = 42; }
+step s1w2 { UPDATE noc SET v = 1 WHERE k = 2; }
+step s1c { COMMIT; }
+
+session s2
+setup { BEGIN ISOLATION LEVEL SERIALIZABLE; }
+step s2r2 { SELECT v FROM noc WHERE k = 2; }
+step s2w1 { UPDATE noc SET v = 42 WHERE k = 1; }
+step s2c { COMMIT; }
+
+# Non-transactional helper: resets the rows before each permutation and reports
+# the final committed state plus a serializability invariant.
+session ctl
+step reset { DELETE FROM noc; INSERT INTO noc VALUES (1, 0), (2, 0); }
+step ck { SELECT k, v FROM noc ORDER BY k;
+ SELECT CASE WHEN (SELECT v FROM noc WHERE k = 1) = 42
+ AND (SELECT v FROM noc WHERE k = 2) = 1
+ THEN 'bad_both_committed'
+ ELSE 'ok_aborted_or_serial' END AS invariant; }
+
+permutation reset s1read s2r2 s1w2 s2w1 s1c s2c ck
+permutation reset s1noop s2r2 s1w2 s2w1 s1c s2c ck
--
2.50.1 (Apple Git-155)
[application/octet-stream] 0002-Predicate-lock-the-conflicting-row-in-INSERT-.-ON-CO.patch (2.8K, 3-0002-Predicate-lock-the-conflicting-row-in-INSERT-.-ON-CO.patch)
download | inline diff:
From 17c2e40d2c2bb8a65fa9e2ca69de8db8cc68ba93 Mon Sep 17 00:00:00 2001
From: Andrey Borodin <[email protected]>
Date: Mon, 1 Jun 2026 12:31:34 +0500
Subject: [PATCH 2/2] Predicate-lock the conflicting row in INSERT ... ON
CONFLICT
Under SERIALIZABLE the arbiter probe reads the conflicting row to decide
the statement's outcome but took no SIREAD lock. When ON CONFLICT then
writes no tuple (DO NOTHING, or DO UPDATE with an unsatisfied WHERE), a
concurrent UPDATE/DELETE of that row missed the rw-antidependency and a
write skew could commit. Lock the tuple where the arbiter finds it.
---
src/backend/executor/execIndexing.c | 32 +++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c
index eb383812901..d49e87a0eda 100644
--- a/src/backend/executor/execIndexing.c
+++ b/src/backend/executor/execIndexing.c
@@ -108,12 +108,14 @@
#include "access/genam.h"
#include "access/relscan.h"
+#include "access/sysattr.h"
#include "access/tableam.h"
#include "access/xact.h"
#include "catalog/index.h"
#include "executor/executor.h"
#include "nodes/nodeFuncs.h"
#include "storage/lmgr.h"
+#include "storage/predicate.h"
#include "utils/injection_point.h"
#include "utils/lsyscache.h"
#include "utils/multirangetypes.h"
@@ -906,9 +908,39 @@ retry:
*/
if (violationOK)
{
+ Datum xminDatum;
+ bool xminIsnull;
+ TransactionId xmin;
+
conflict = true;
if (conflictTid)
*conflictTid = existing_slot->tts_tid;
+
+ /*
+ * This conflicting row determined the outcome of the INSERT ...
+ * ON CONFLICT, so for serializability it was read just as a SELECT
+ * of it would be. Record an SIREAD lock so that a concurrent
+ * modification of the row creates the necessary rw-antidependency,
+ * even when ON CONFLICT writes no tuple (DO NOTHING, or DO UPDATE
+ * with an unsatisfied WHERE). A no-op outside SERIALIZABLE.
+ *
+ * XXX A reviewer familiar with predicate.c should confirm the
+ * xmin handling here. slot_getsysattr() returns the raw xmin
+ * (HeapTupleHeaderGetRawXmin), while the other PredicateLockTID()
+ * call sites pass the frozen-aware HeapTupleHeaderGetXmin(). This
+ * value only feeds the "did this xact write the tuple" early-out
+ * in PredicateLockTID(), and a frozen tuple can never belong to the
+ * current transaction, so the raw value should be equivalent here
+ * -- but the inconsistency with the other call sites is worth a
+ * second look.
+ */
+ xminDatum = slot_getsysattr(existing_slot,
+ MinTransactionIdAttributeNumber,
+ &xminIsnull);
+ Assert(!xminIsnull);
+ xmin = DatumGetTransactionId(xminDatum);
+ PredicateLockTID(heap, &existing_slot->tts_tid,
+ estate->es_snapshot, xmin);
break;
}
--
2.50.1 (Apple Git-155)
view thread (9+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected]
Subject: Re: Possible G2-item at SERIALIZABLE
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox