On 12 Dec 2025 at 07:15 +0100, jian he <jian(dot)universality(at)gmail(dot)com>, wrote:
> On Sat, Nov 29, 2025 at 6:02 AM Viktor Holmberg <v(at)viktorh(dot)net> wrote:
> >
> > Attaching v18 with the above changes. Thanks your continued reviews Jian!
> >
> hi.
> I had some minor comments with doc, comments.
Thanks, all changed now.
> + if (lockStrength == LCS_NONE)
> + {
> + if (!table_tuple_fetch_row_version(relation, conflictTid,
> SnapshotAny, existing))
> + /* The pre-existing tuple was deleted */
> + return false;
> + }
> i think this part should be
> ```
> if (!table_tuple_fetch_row_version(rel, conflictTid, SnapshotAny, existing))
> elog(ERROR, "failed to fetch conflicting tuple for ON CONFLICT");
> ```
>
> say we have a conflict for values (1)
> ``insert into tsa values (1,3) on conflict(a) do select returning *;``
>
> set a GDB breakpoint at ExecOnConflictSelect, let another process do
> ``delete from tsa; vacuum tsa;``
> then let GDB continue.
>
> table_tuple_fetch_row_version can still fetch the tuple.
> so I think this is an unlikely scenario.
Ok, I find this change slightly scary, but I’ve now changed this to assert that table_tuple_fetch_row_version is true. You say “unlikely” but having looked at it for a while I can’t see any case where it’d happen. Hence an assert seems most appropriate. I was thinking about asserting it even before the if as I believe the tuple should always be physically present, but I didn’t dare to. If anyone can think of a case where it’d happen I’d love to hear it!
/Viktor