Peter Geoghegan <p...@heroku.com> writes: > On Sat, Oct 22, 2016 at 9:38 AM, Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers