Re: On conflict update & hint bits

Peter Geoghegan <pg(at)heroku(dot)com> writes:
> Attached is a revision of Thomas' patch to fix a related issue; it now
> also fixes this no-buffer-lock-held issue.

I think this needs more work.

1. Why are you replacing ExecOnConflictUpdate's ExecCheckHeapTupleVisible
call with ExecCheckTIDVisible? That appears to demand a re-fetch of the
tuple ExecOnConflictUpdate already has its hands on. Wouldn't it be
better to just put a buffer lock/unlock around the existing code?

2. Your proposed coding

+ if (!heap_fetch(rel, estate->es_snapshot, &tuple, &buffer, true, NULL))
+ ...
+ if (!TransactionIdIsCurrentTransactionId(HeapTupleHeaderGetXmin(tuple.t_data)))

will dump core in the case where heap_fetch returns false with
tuple.t_data set to null. If that case is impossible here,
it's not apparent why, and it'd surely deserve at least a comment,
maybe an Assert. But I'd rather just assume it's possible.

I'm not taking a position on whether this is OK at the higher level
of whether the SSI behavior could be better. However, unless Kevin
has objections to committing something like this now, I think we
should fix the above-mentioned problems and push it. The existing
behavior is clearly bad.

regards, tom lane