SSI: ON CONFLICT DO SELECT takes no predicate lock on the returned row

Hi,

On 2026-08-19 12:33:01 +0100, Zsolt Parragi wrote:

While testing the new INSERT...ON CONFLICT DO SELECT feature I found a
possible serialization violation: the row returned back by DO SELECT
is not covered by any SIREAD lock, so a concurrent transaction can
modify it.

Good catch.

@@ -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);
+	}
+

I don't think this is quite right though. The details of how heapam uses SI
locks is heapam specific and should live in the heapam code. I also think you
actually need to acquire the predicate lock before ExecOnConflictLockRow()?

Greetings,

Andres Freund

Good catch.

Look like we also had a real-life serialization issue, this was
already mentioned and fixed differently by Andrey in another thread
[1]: /messages/by-id/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru
was reviewing at the same time, and I didn't notice that Andrey sent
another email that day.

His patch also handles a similar issue in DO UPDATE / DO NOTHING which
my patch didn't address.

I don't think this is quite right though. The details of how heapam uses SI
locks is heapam specific and should live in the heapam code. I also think you
actually need to acquire the predicate lock before ExecOnConflictLockRow()?

You are right, and after looking at Andrey's patch I think that it
also might have a similar but smaller race window, so I'll still
attach this v2 as it tries a different direction. This v2 still only
addresses the DO SELECT part.

Please also take a look at the patches in the other thread, both for
this issue and the other more corner-case issue we were discussing
there.

[1]: /messages/by-id/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru

On 19 Aug 2026, at 22:32, Zsolt Parragi <zsolt.parragi@percona.com> wrote:

<v2-0001-Take-SIREAD-lock-on-rows-read-by-ON-CONFLICT-DO-S.patch>

Hi Zsolt and Andres,

Yes, I think this is another manifestation of the same underlying issue.
My June patch happened to cover DO SELECT too, because it put the SIREAD
lock in the common arbiter probe, although the email only mentioned DO
NOTHING and DO UPDATE with a false WHERE clause.

However, I agree that the June fix has the race you pointed out. A
writer can pass its conflict-in check between the dirty arbiter probe
and PredicateLockTID(). The MVCC fetch in v2 handles both directions:
it either installs the SIREAD lock before a future writer, or detects a
writer that has already modified the tuple. This looks like the right
approach to me.

The commit message explains why the fetch must precede FOR KEY SHARE,
but the test uses FOR UPDATE. Could we test FOR KEY SHARE with a
non-key UPDATE instead? That would distinguish this ordering from
taking the SIREAD lock after the tuple lock.

It would also be useful to cover DO SELECT WHERE false. The conflicting
row is not returned in that case, but it still determined the outcome of
the INSERT and was examined by the WHERE clause.

After that, I think we should adapt the same approach to the DO NOTHING
and filtered DO UPDATE cases in the older branches.

Thank you!

Best regards, Andrey Borodin.

Does this one deserve a mention on the open items wiki [0]https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items?

[0]: https://wiki.postgresql.org/wiki/PostgreSQL_19_Open_Items

--
nathan

Thanks for the review!

v3 adds additional test cases, otherwise it is unchanged.

Does this one deserve a mention on the open items wiki

Probably it should be there, since it's about a pg19 feature.

On 26 Aug 2026, at 03:38, Zsolt Parragi <zsolt.parragi@percona.com> wrote:

<v3-0001-Take-SIREAD-lock-on-rows-read-by-ON-CONFLICT-DO-S.patch>

I think the patch is Ready for Committer.

Best regards, Andrey Borodin.

Adding Dean Rasheed to the thread, since he committed this feature. Please
note that this is marked as an open item for v19.

--
nathan

On Thu, 3 Sept 2026 at 14:31, Nathan Bossart <nathandbossart@gmail.com> wrote:

Adding Dean Rasheed to the thread, since he committed this feature. Please
note that this is marked as an open item for v19.

I started looking at this, and it looks to me like another
manifestation of the pre-existing bug with ON CONFLICT DO NOTHING / DO
UPDATE reported in [1]/messages/by-id/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru.

I tried out the ON CONFLICT DO NOTHING case, using the test case added
in [1]/messages/by-id/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru, which defines a table as follows, initially containing 2 rows:

CREATE TABLE noc (k int PRIMARY KEY, v int);
INSERT INTO noc VALUES (1, 0), (2, 0);

and then 2 transactions are executed concurrently in 2 sessions:

S1: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
S2: BEGIN TRANSACTION ISOLATION LEVEL SERIALIZABLE;
S1: INSERT INTO noc(k, v) VALUES (1, 99) ON CONFLICT (k) DO NOTHING;
S2: SELECT v FROM noc WHERE k = 2;
S1: UPDATE noc SET v = 1 WHERE k = 2;
S2: DELETE FROM noc WHERE k = 1;
S1: COMMIT;
S2: COMMIT;

Running this on HEAD, the SELECT returns 0, both transactions commit
successfully, and the table ends up containing just 1 row, with k = 2
and v = 1. That result isn't consistent with either ordering of those
2 transactions, so I concur that this is a genuine SSI bug -- one of
the transactions should have been aborted.

Given that, I think the best approach would be to fix this closer to
the underlying cause, which I think is the absence of an SIREAD lock
when probing the arbiter index, rather than adopting the fix suggested
in this thread, which only fixes the ON CONFLICT DO SELECT case.

I tested the fix from [1]/messages/by-id/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru, and can confirm that it fixes the ON
CONFLICT DO SELECT bug as well as the ON CONFLICT DO NOTHING / DO
UPDATE bugs. However, that fix seems to have the same kind of layering
violation that Andres complained about for the first version of the
patch on this thread -- predicate locking should be in heapam code,
not in execIndexing.c.

So perhaps what we need to do (borrowing elements from both patches)
is re-fetch the existing tuple using table_tuple_fetch_row_version()
with estate->es_snapshot in check_exclusion_or_unique_constraint(), so
that we take an SIREAD lock regardless of what conflict action is
executed.

That's my initial take, anyway. I haven't tried that yet.

Regards,
Dean

[1]: /messages/by-id/787936C5-4155-4CF9-939D-39DC0EC1C892@yandex-team.ru

On Mon, 07 Sep 2026, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

However, that fix seems to have the same kind of layering
violation that Andres complained about for the first version of the
patch on this thread -- predicate locking should be in heapam code,
not in execIndexing.c.

It's not just the layering violation, that solution also has a leftover issue, similarly to my first patch in this thread. When I looked at this my initial conclusion was that we can't fix the two at the same place without layering violations / remaining issues, so my idea was to apply the same fix as I did for SELECT to those paths, but I'll take another look at it.

However, that fix seems to have the same kind of layering
violation that Andres complained about for the first version of the
patch on this thread -- predicate locking should be in heapam code,
not in execIndexing.c.

It's not just the layering violation, that solution also has a leftover issue, similarly to my first patch in this thread. When I looked at this my initial conclusion was that we can't fix the two at the same place without layering violations / remaining issues, so my idea was to apply the same fix as I did for SELECT to those paths, but I'll take another look at it.

And I was wrong, this approach is simple and works for all reported
cases, I attached v4 which uses this approach.

Hi Zsolt,

On 10 Sep 2026, Zsolt Parragi wrote:

this approach is simple and works for all reported cases

I reviewed v4 and it looks correct to me. I also replaced the MVCC fetch
with just PredicateLockTID(): the ordinary isolation test still passed,
but all three probe-window permutations allowed both transactions to
commit. The new test catches the race in my June approach.

I checked the DO NOTHING and filtered DO UPDATE cases on REL_14_STABLE
too, using the same code change without the injection point. All three
permutations allow both transactions to commit without the fix, and
produce the expected serialization failures with it.

Thank you!

Best regards, Andrey Borodin.

On Thu, 10 Sept 2026 at 09:45, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

On 10 Sep 2026, Zsolt Parragi wrote:

this approach is simple and works for all reported cases

Thanks for the update. The new patch looks good.

I reviewed v4 and it looks correct to me. I also replaced the MVCC fetch
with just PredicateLockTID(): the ordinary isolation test still passed,
but all three probe-window permutations allowed both transactions to
commit. The new test catches the race in my June approach.

It looks correct to me too. It's a nice touch using injection points
to confirm that there's no longer an issue with the window between
probe and re-fetch.

Nitpicking, I'd say that it's better to name the steps in the tests
things like "ioc_nothing1" and "ioc_update1_where", so that they're
more distinct from steps like "update2", which is just a plain update.

Other than that, I'd say that it's ready to commit, but I'll take
another couple of days to think it over more thoroughly, before
pushing it (probably at the weekend).

Regards,
Dean

On Thu, 10 Sept 2026 at 11:24, Dean Rasheed <dean.a.rasheed@gmail.com> wrote:

I'll take
another couple of days to think it over more thoroughly, before
pushing it (probably at the weekend).

I've been thinking about this some more, and I think that it's
possible to fix this without a potentially expensive re-fetch.

The attached v5 patch modifies the predicate locking code so that it
takes predicate locks during the arbiter index probe, even though it's
still using a dirty snapshot, so the read is recorded during the
probe, as it would be for an MVCC index scan.

It does this using a new special snapshot, SnapshotDirtySerializable,
which is a dirty snapshot, obeying the same tuple visibility rules,
but is special-cased for predicate locking. Technically, this is not
properly re-entrant safe, since it's a global datastructure, but I
think if user-defined operators or index expressions are recursively
calling INSERT ... ON CONFLICT here, then there are bigger problems
than getting incorrect serialization results.

This approach eliminates any window between probe and re-fetch (there
is no re-fetch), so there's no need to worry about that gap, which
makes the patch simpler, while still passing all the other tests.

Regards,
Dean

Hi Dean,

On 12 Sep 2026, Dean Rasheed wrote:

possible to fix this without a potentially expensive re-fetch.

Unlike v4, v5 also takes SIREAD locks on index pages, which can cause
extra false positives. The additional retries could outweigh saving
the fetch.

Also, violationOK includes preliminary checks of deferred exclusion
constraints, so v5 adds predicate locking there at SERIALIZABLE too.
Is that needed? Could we restrict the new snapshot to arbiter probes?

Thank you!

Best regards, Andrey Borodin.

On Sat, 12 Sept 2026 at 19:01, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

On 12 Sep 2026, Dean Rasheed wrote:

possible to fix this without a potentially expensive re-fetch.

Unlike v4, v5 also takes SIREAD locks on index pages, which can cause
extra false positives. The additional retries could outweigh saving
the fetch.

Yes, I've been wondering about that. My first thought was that this is
necessary to prevent phantom reads, but after thinking about it some
more, I'm pretty sure that it's not necessary, and it's sufficient to
just lock the heap tuple, because of the way IOC works.

Also, violationOK includes preliminary checks of deferred exclusion
constraints, so v5 adds predicate locking there at SERIALIZABLE too.
Is that needed? Could we restrict the new snapshot to arbiter probes?

Yeah, I dont' think it's needed. v4 also takes a predicate lock for
deferred exclusion constraints, but obviously only in the case where a
potential conflict is found, whereas v5 is doing it even in cases
where there is no conflict.

So unless anyone thinks otherwise, v4 is probably better than v5, and
I'll push that.

Regards,
Dean

Hi Dean,

On 13 Sep 2026, Dean Rasheed wrote:

v4 also takes a predicate lock for
deferred exclusion constraints

The fetch is inside if (conflictTid), and the deferred exclusion checks
pass NULL, so they do not acquire additional predicate locks IIUC.

+1 to v4

Thank you!

Best regards, Andrey Borodin.

On Sun, 13 Sept 2026 at 16:14, Andrey Borodin <x4mmm@yandex-team.ru> wrote:

+1 to v4

OK, I've pushed v4. Thank you both.

Regards,
Dean