Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
Stephen Frost wrote: > 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. FWIW I think the translatability argument carries very little weight. We add new messages to the catalog in minor releases every now and then. We don't take it lightly of course, but avoiding so isn't a reason to not fix a bug properly. In this case, given the discussion, it seems to me that the right fix is to create a new XLTW enum as I already mentioned, with a message tailored to the specific case. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
On Mon, Mar 21, 2016 at 3:38 PM, Stephen Frost wrote: >> 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? Because it's XLTW_InsertIndex in check_exclusion_or_unique_constraint() now, this is the message: case XLTW_InsertIndex: cxt = gettext_noop("while inserting index tuple (%u,%u) in relation \"%s\""); break; I guess what's at issue is whether or not it's okay that the (heap) tuple TID shown (by this message) when waiting in check_exclusion_or_unique_constraint() during ON CONFLICT is *not* our own -- rather, it's the other guy's for this new use of XLTW_InsertIndex. Does the message itself convey a contrary meaning, or is it ambiguous in a way that supports either interpretation (i.e. does that ambiguity allow us to leave things as they are)? Certainly, this is unlike the nbtinsert.c XLTW_InsertIndex case (the only other instance of XLTW_InsertIndex), where we must have physically inserted already, and report specifically on our own physically inserted TID when this message is shown. Does that inconsistency alone make this wrong? I think that XLTW_InsertIndex is intended to mean our own TID, based on .1) the only precedent we have, and .2) based on the fact that there is a distinct XLTW_InsertIndexUnique case at all. BTW, just so the difference is clear, I point out that XLTW_InsertIndexUnique (which I now propose to change check_exclusion_or_unique_constraint() to use) says: case XLTW_InsertIndexUnique: cxt = gettext_noop("while checking uniqueness of tuple (%u,%u) in relation \"%s\""); break; > 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). I see your point. That's what I thought, initially. >> 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. Well, we're determining if there was a conflict or not, in the first phase of speculative insertion. That means that we need to see if an update is required (for example). But that needs to behave just like checking the uniqueness of an existing thing. It's just that our own not-yet-physically-inserted "conceptual" tuple needs to be the thing that makes this existing tuple (from some other xact) non-unique. If that sounds weird, consider that in the standard, non-speculative exclusion constraint case, the physical insertion actually has already occurred, but even still we actually are waiting on the other guy's tuple/xact (and report the other guy's TID, not our own, unlike with nbtinsert.c). Notice that we make sure that it's another XID towards the top of the loop within check_exclusion_or_unique_constraint()). And so, its message says: case XLTW_RecheckExclusionConstr: cxt = gettext_noop("while checking exclusion constraint on tuple (%u,%u) in relation \"%s\""); break; Of course, that this appeared for ON CONFLICT DO NOTHING with a B-Tree index in 9.5.1 was wrong, but only because it said "exclusion constraint" rather than "unique index". That's about what XLTW_InsertIndexUnique says already, which is why I now think we should just use XLTW_InsertIndexUnique. > 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. Right, but I don't think that XLTW_InsertIndexUnique specifically implies that we're not inserting, just as XLTW_RecheckExclusionConstr does not specifically imply that we're not inserting (actually, we're usually or always inserting with XLTW_RecheckExclusi
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
Peter, * Peter Geoghegan (p...@heroku.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 signature.asc Description: Digital signature
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
On Tue, Mar 15, 2016 at 8:31 AM, Stephen Frost wrote: >> > We wouldn't want to end up returning different error messages for the >> > same command under the same conditions just based, which is what we'd >> > potentially end up doing if we used XLTW_InsertIndexUnique here. >> >> Perhaps we need a new value in that enum, so that the context message is >> specific to the situation at hand? > > Maybe, but that would require a new message and new translation and just > generally doesn't seem like something we'd want to back-patch for a > bugfix. 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. 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. 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). 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. Sorry about that. My confusion came from the fact that historically, when check_exclusion_or_unique_constraint() was called check_exclusion_constraint(), it (almost) was our own tuple that was waited on. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
On Tue, Mar 15, 2016 at 6:18 AM, Stephen Frost wrote: > Agreed. I'm going to play with it a bit more but barring objections, > I'll commit and back-patch Peter's patch. Thanks for taking care of this, Stephen. -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
* Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Julien Rouhaud (julien.rouh...@dalibo.com) wrote: > > > XLTW_InsertIndexUnique is used when building a unique index, but this is > > just a check, and more to the point, it's actually a re-check of what > > we're doing in nbinsert.c where we're already using XLTW_InsertIndex. > > > > We wouldn't want to end up returning different error messages for the > > same command under the same conditions just based, which is what we'd > > potentially end up doing if we used XLTW_InsertIndexUnique here. > > Perhaps we need a new value in that enum, so that the context message is > specific to the situation at hand? Maybe, but that would require a new message and new translation and just generally doesn't seem like something we'd want to back-patch for a bugfix. As such, if someone's interested, they could certainly propose it to go into HEAD, but I'm not going to look at making those kinds of changes for this bugfix. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
Stephen Frost wrote: > * Julien Rouhaud (julien.rouh...@dalibo.com) wrote: > XLTW_InsertIndexUnique is used when building a unique index, but this is > just a check, and more to the point, it's actually a re-check of what > we're doing in nbinsert.c where we're already using XLTW_InsertIndex. > > We wouldn't want to end up returning different error messages for the > same command under the same conditions just based, which is what we'd > potentially end up doing if we used XLTW_InsertIndexUnique here. Perhaps we need a new value in that enum, so that the context message is specific to the situation at hand? -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
On 15/03/2016 14:18, Stephen Frost wrote: > * Julien Rouhaud (julien.rouh...@dalibo.com) wrote: >> On 15/03/2016 03:30, Peter Geoghegan wrote: >>> On Mon, Mar 7, 2016 at 1:46 PM, Peter Geoghegan wrote: Attached patch fixes a bug reported privately by Stephen this morning. >>> >>> Bump. >>> >>> I would like to see this in the next point release. It shouldn't be >>> hard to review. >>> >> >> +reason_wait = indexInfo->ii_ExclusionOps ? >> +XLTW_RecheckExclusionConstr : XLTW_InsertIndex; >> >> Shouldn't it be set to XLTW_InsertIndexUnique instead? > > Actually, no, though I had the same question for Peter when I was first > reviewing this. > > XLTW_InsertIndexUnique is used when building a unique index, but this is > just a check, and more to the point, it's actually a re-check of what > we're doing in nbinsert.c where we're already using XLTW_InsertIndex. > > We wouldn't want to end up returning different error messages for the > same command under the same conditions just based, which is what we'd > potentially end up doing if we used XLTW_InsertIndexUnique here. > Oh I see. Thanks for the explanation! >> Otherwise the patch seems ok to me. > > Agreed. I'm going to play with it a bit more but barring objections, > I'll commit and back-patch Peter's patch. > > Thanks! > > Stephen > -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
* Julien Rouhaud (julien.rouh...@dalibo.com) wrote: > On 15/03/2016 03:30, Peter Geoghegan wrote: > > On Mon, Mar 7, 2016 at 1:46 PM, Peter Geoghegan wrote: > >> Attached patch fixes a bug reported privately by Stephen this morning. > > > > Bump. > > > > I would like to see this in the next point release. It shouldn't be > > hard to review. > > > > + reason_wait = indexInfo->ii_ExclusionOps ? > + XLTW_RecheckExclusionConstr : XLTW_InsertIndex; > > Shouldn't it be set to XLTW_InsertIndexUnique instead? Actually, no, though I had the same question for Peter when I was first reviewing this. XLTW_InsertIndexUnique is used when building a unique index, but this is just a check, and more to the point, it's actually a re-check of what we're doing in nbinsert.c where we're already using XLTW_InsertIndex. We wouldn't want to end up returning different error messages for the same command under the same conditions just based, which is what we'd potentially end up doing if we used XLTW_InsertIndexUnique here. > Otherwise the patch seems ok to me. Agreed. I'm going to play with it a bit more but barring objections, I'll commit and back-patch Peter's patch. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
On 15/03/2016 03:30, Peter Geoghegan wrote: > On Mon, Mar 7, 2016 at 1:46 PM, Peter Geoghegan wrote: >> Attached patch fixes a bug reported privately by Stephen this morning. > > Bump. > > I would like to see this in the next point release. It shouldn't be > hard to review. > + reason_wait = indexInfo->ii_ExclusionOps ? + XLTW_RecheckExclusionConstr : XLTW_InsertIndex; Shouldn't it be set to XLTW_InsertIndexUnique instead? Otherwise the patch seems ok to me. > Thanks > -- Julien Rouhaud http://dalibo.com - http://dalibo.org -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
On Mon, Mar 7, 2016 at 1:46 PM, Peter Geoghegan wrote: > Attached patch fixes a bug reported privately by Stephen this morning. Bump. I would like to see this in the next point release. It shouldn't be hard to review. Thanks -- Peter Geoghegan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Minor bug affecting ON CONFLICT lock wait log messages
Attached patch fixes a bug reported privately by Stephen this morning. He complained about deadlocking ON CONFLICT DO NOTHING statements. There were no exclusion constraints involved, and yet they were incorrectly indicated as being involved in log messages that related to these deadlocks. -- Peter Geoghegan From bc481af77994057cb1ffe4a0e471b38bb00dc228 Mon Sep 17 00:00:00 2001 From: Peter Geoghegan Date: Mon, 7 Mar 2016 13:16:24 -0800 Subject: [PATCH] Avoid incorrectly indicating exclusion constraint wait INSERT ... ON CONFLICT's precheck may have to wait on the outcome of another insertion, which may or may not itself be a speculative insertion. This wait is not necessarily associated with an exclusion constraint, but was always reported that way in log messages if the wait happened to involve a tuple that had no speculative token. Bug reported privately by Stephen Frost. His case involved ON CONFLICT DO NOTHING, where spurious references to exclusion constraints in log messages were more likely. --- src/backend/executor/execIndexing.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/backend/executor/execIndexing.c b/src/backend/executor/execIndexing.c index 838cee7..5d553d5 100644 --- a/src/backend/executor/execIndexing.c +++ b/src/backend/executor/execIndexing.c @@ -725,6 +725,7 @@ retry: { TransactionId xwait; ItemPointerData ctid_wait; + XLTW_Oper reason_wait; Datum existing_values[INDEX_MAX_KEYS]; bool existing_isnull[INDEX_MAX_KEYS]; char *error_new; @@ -783,13 +784,14 @@ retry: TransactionIdPrecedes(GetCurrentTransactionId(), xwait { ctid_wait = tup->t_data->t_ctid; + reason_wait = indexInfo->ii_ExclusionOps ? +XLTW_RecheckExclusionConstr : XLTW_InsertIndex; index_endscan(index_scan); if (DirtySnapshot.speculativeToken) SpeculativeInsertionWait(DirtySnapshot.xmin, DirtySnapshot.speculativeToken); else -XactLockTableWait(xwait, heap, &ctid_wait, - XLTW_RecheckExclusionConstr); +XactLockTableWait(xwait, heap, &ctid_wait, reason_wait); goto retry; } -- 1.9.1 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers