Re: Minor bug affecting ON CONFLICT lock wait log messages

Peter,

* Peter Geoghegan (pg(at)heroku(dot)com) wrote:
> Thinking about this again, I think we should use
> XLTW_InsertIndexUnique after all. The resemblance of the
> check_exclusion_or_unique_constraint() code to the nbtinsert.c code
> seems only superficial on second thought. So, I propose fixing the fix
> by changing XLTW_InsertIndex to XLTW_InsertIndexUnique.

I'm not against changing it, but...

> Basically, unlike with the similar nbtinsert.c code, we're checking
> someone else's tuple in the speculative insertion
> check_exclusion_or_unique_constraint() case that was changed (or it's
> an exclusion constraint, where even the check for our own tuple
> happens only after insertion; no change there in any case). Whereas,
> in the nbtinsert.c case that I incorrectly duplicated, we're
> specifically indicating that we're waiting on *our own* already
> physically inserted heap tuple, and say as much in the
> XLTW_InsertIndex message that makes it into the log.

I don't quite follow how we're indicating that we're waiting on our own
already physically inserted heap tuple in the log?

> So, the
> fd658dbb300456b393536802d1145a9cea7b25d6 fix is wrong in that we now
> indicate that the wait was on our own already-inserted tuple, and not
> *someone else's* already-inserted tuple, as it should (we haven't
> inserting anything in the first phase of speculative insertion, this
> precheck). This code is not a do-over of the check in nbtinsert.c --
> rather, the code in nbtinsert.c is a second phase do-over of this code
> (where we've physically inserted a heap tuple + index tuple --
> "speculative" though that is).

One of the reasons I figured the XLTW_InsertIndex message made sense in
this case is that it'd provide a consistent result for users.
Specifically, I don't think it makes sense to produce different messages
based only on who got there first. I don't mind having a different
message when there's something different happening (there's exclusion
constraints involved, or you're building an index, etc).

> It seems fine to characterize a wait here as "checking the uniqueness
> of [somebody else's] tuple", even though technically we're checking
> the would-be uniqueness were we to (speculatively, or actually)
> insert. However, it does not seem fine to claim ctid_wait as a tuple
> we ourselves inserted.

I don't think either message really fits here, unfortunately. We're not
actually checking the uniqueness of someone else's tuple here either,
after all, we're waiting to see what happens with their tuple because
ours won't be unique if it goes in with that other tuple in place, if
I'm following along correctly.

One thing I can say is that the XLTW_InsertIndex at least matches the
*action* we're taking, which is that we're trying to INSERT. While
having the ctid included in the message may be useful for debugging,
I've got doubts about including it in regular messages like this; we
certainly don't do that for the 'normal' INSERT case when we discover
a duplicate key, but that ship has sailed.

Ultimately, I guess I'm inclined to leave it as-committed. If you
understand enough to realize what that pair of numbers after 'tuple'
means, you've probably found this thread and followed it enough to
understand what's happening.

I don't feel terribly strongly about that position and so if others
feel the XLTW_InsertIndexUnique message really would be better, I'd be
happy to commit the change.

Thanks!

Stephen