On 12 Dec 2025 at 07:15 +0100, jian he <jian.universality@gmail.com>, wrote:
On Sat, Nov 29, 2025 at 6:02 AM Viktor Holmberg <v@viktorh.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
