SSI: ON CONFLICT DO SELECT takes no predicate lock on the returned row
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t253491psql -h localhost -U postgresBuilt from patchset v3 (message #3), August 23, 2026 at 07:46 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t253491_3 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t253491_3 && git checkout t253491_3Patchset v3 (message #3) is on t253491_3
Hello!
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.
s1: INSERT INTO a VALUES (1,99) ON CONFLICT (key) DO SELECT RETURNING val;
s2: SELECT count(*) FROM b;
s2: UPDATE a SET val = 1 WHERE key = 1;
s1: INSERT INTO b VALUES (1,10);
s1: COMMIT;
s2: COMMIT;
And both transactions commit. If you replace the DO SELECT with a
plain SELECT, or SELECT FOR UPDATE s2 fails with a serialization
error, as expected.
The attached patch takes a predicate lock to fix the issue and adds an
isolation test covering the above example.
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.