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
