Re: On conflict update & hint bits

On Sun, Oct 23, 2016 at 1:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> "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.

The heap_fetch() contract doesn't ask its callers to consider any of
that. Besides, those actions (PredicateLockTuple +
CheckForSerializableConflictOut) are very probably redundant no-ops as
you say. (They won't help with Kevin's additional concerns, BTW,
because Kevin is concerned about excessive serialization failures with
SSI.)

> 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.

I'm surprised at how you've assessed the risk here. I think that the
risk of not proceeding with fixing the buffer lock issue is greater
than the risk of breaking something else with the proposed fix. Even
if the former risk isn't such a big one.

If there are a non-trivial number of users that are particularly
attached to the precise behavior of higher isolation levels in master
(assuming it would be altered by the proposed fix), then it's
surprising that it took so many months for someone to complain about
the ON CONFLICT DO NOTHING behavior being blatantly broken.

-- 
Peter Geoghegan