Peter Geoghegan <pg@heroku.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
