Peter Geoghegan <pg@heroku.com> writes:
> On Sat, Oct 22, 2016 at 9:38 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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?
> I thought that that was less than idiomatic within nodeModifyTable.c
> -- executor modules rarely directly acquire buffer locks.
"Rarely" is not "never". A bigger problem though is that heap_fetch
does more than just lock the buffer: there are also PredicateLockTuple
and CheckForSerializableConflictOut calls in there. It's possible that
those are no-ops in this usage (because after all we already fetched
the tuple once), or maybe they're even desirable because they would help
resolve Kevin's concerns. But it hasn't been analyzed and so I'm not
prepared to risk a behavioral change in this already under-tested area
the day before a release wrap.
AFAICT, it's very hard to get to an actual failure from the lack of
buffer lock here. You would need to have a situation where the tuple
was not hintable when originally fetched but has become so by the time
ON CONFLICT is rechecking it. That's possible, eg if we're using
async commit and the previous transaction's commit record has gotten
flushed to disk in between, but it's not likely. Even then, no actual
problem would ensue (in non-assert builds) from setting a hint bit without
buffer lock, except in very hard-to-hit race conditions. So the buffer
lock issue doesn't seem urgent enough to me to justify making a fix under
time pressure.
The business about not throwing a serialization failure due to actions
of our own xact does seem worth fixing now, but if I understand correctly,
we can deal with that by adding a test for xmin-is-our-own-xact into
the existing code structure. I propose doing that much (and adding
Munro's regression test case) and calling it good for today.
regards, tom lane