Обсуждение: Small locking bugs in hs
Hi, While unlikely to cause issues two new LWLockAcquire calls use the wrong locking mode. Patch attached. Andres
On Sun, 2009-12-27 at 20:12 +0100, Andres Freund wrote: > While unlikely to cause issues two new LWLockAcquire calls use the wrong > locking mode. > Patch attached. It's important to explain why you think something is a bug, rather than make that claim on its own. The locking mode was intentional, though not documented either way. However, I think I see a different issue with conflict handling, so looking at this again was worthwhile, thanks. Will come back with more over next few days. -- Simon Riggs www.2ndQuadrant.com
On Sunday 27 December 2009 23:10:09 Simon Riggs wrote: > On Sun, 2009-12-27 at 20:12 +0100, Andres Freund wrote: > > While unlikely to cause issues two new LWLockAcquire calls use the wrong > > locking mode. > > Patch attached. > It's important to explain why you think something is a bug, rather than > make that claim on its own. Youre right. My idea was that another SIGINT handler (e.g. normal client query cancel) is running while CONFLICT_MODE_FATAL is issued that might get ignored. Thus the fatal error might get ignored. But actually that was mostly my gut feeling - I am not really understanding the whole query cancellation process yet. (Its not exactly unlikely that my patch does not fix that though) I think that if under protection of a shared lock the protected objects gets altered, that definitely needs to be commented from my pov... In the reverse case its not as important but still a good idea. > However, I think I see a different issue with conflict handling, so > looking at this again was worthwhile, thanks. Will come back with more > over next few days. Cool. Andres
Hi Simon, Btw, dont understand my questions as criticism or such. I am really impressed by the quality of the HS patch - many thanks to you, heikki and all the others. Andres
On Mon, 2009-12-28 at 01:22 +0100, Andres Freund wrote: > Btw, dont understand my questions as criticism or such. I didn't take them that way. Your questions and bug reports are welcome. It was important that HS was released in Alpha so that we can shake out bugs, issues and concerns early enough to get as many of them fixed in this release as possible. It is also important that I fix remaining issues in priority order. -- Simon Riggs www.2ndQuadrant.com
On Sun, Dec 27, 2009 at 2:12 PM, Andres Freund <andres@anarazel.de> wrote: > While unlikely to cause issues two new LWLockAcquire calls use the wrong > locking mode. > Patch attached. Does it make sense to add this to the 2010-01 CommitFest so we don't lose track of it? ...Robert
On Thu, 2010-01-07 at 13:16 -0500, Robert Haas wrote: > On Sun, Dec 27, 2009 at 2:12 PM, Andres Freund <andres@anarazel.de> wrote: > > While unlikely to cause issues two new LWLockAcquire calls use the wrong > > locking mode. > > Patch attached. > > Does it make sense to add this to the 2010-01 CommitFest so we don't > lose track of it? No. As mentioned upthread, this is not a bug. I also mentioned having a separate, related thought about this, which has already been handled. There are no further actions required on this. -- Simon Riggs www.2ndQuadrant.com
On Saturday 16 January 2010 12:32:35 Simon Riggs wrote: > On Thu, 2010-01-07 at 13:16 -0500, Robert Haas wrote: > > On Sun, Dec 27, 2009 at 2:12 PM, Andres Freund <andres@anarazel.de> wrote: > > > While unlikely to cause issues two new LWLockAcquire calls use the > > > wrong locking mode. > > > Patch attached. > > > > Does it make sense to add this to the 2010-01 CommitFest so we don't > > lose track of it? > > No. As mentioned upthread, this is not a bug. Could you also mention in a littlebit more detail why not? Thanks, Andres
On Wed, 2010-01-20 at 04:47 +0100, Andres Freund wrote: > On Saturday 16 January 2010 12:32:35 Simon Riggs wrote: > > > > No. As mentioned upthread, this is not a bug. > Could you also mention in a little bit more detail why not? When a cleanup record arrives without a latestRemovedXid we are forced to assume that the xid could be as late as latestCompletedXid. Regrettably we aren't certain which of the xids are still there since it is possible that earlier xids in KnownAssignedXids are actually FATAL errors that did not write abort records. So we need to conflict with all current snapshots whose xmin is less than latestCompletedXid to be safe. This can cause false positives in our assessment of which vxids conflict. By using exclusive lock we prevent new snapshots from being taken while we work out which snapshots to conflict with. This protects those new snapshots from also being included in our conflict list. After the lock is released, we allow snapshots again. It is possible that we arrive at a snapshot that is identical to one that we just decided we should conflict with. This a case of false positives, not an actual problem. There are two cases: (1) if we were correct in using latestCompletedXid then that means that all xids in the snapshot lower than that are FATAL errors, so not xids that ever commit. We can make no visibility errors if we allow such xids into the snapshot. (2) if we erred on the side of caution and in fact the latestRemovedXid should have been earlier than latestCompletedXid then we conflicted with a snapshot needlessly. Taking another identical snapshot is OK, because the earlier conflicted snapshot was a false positive. In either case, a snapshot taken after conflict assessment will still be valid and non-conflicting even if an identical snapshot that existed before conflict assessment was assessed as conflicting. If we allowed concurrent snapshots while we were deciding who to conflict with we would need to include all concurrent snapshotters in the conflict list as well. We'd have difficulty in working out exactly who that was, so it is happier for all concerned if we take an exclusive lock. It also means that users waiting for a snapshot is a good thing, since it is more likely that they will live longer after having waited. So its not a bug for us to use exclusive lock and is actually desirable. We could reduce false positives by having the master calculate the exact xmin each time it issues an XLOG_BTREE_DELETE record. That would introduce more contention since that happens during btree split operations, so might be counter productive. -- Simon Riggs www.2ndQuadrant.com
On Wednesday 20 January 2010 12:59:40 Simon Riggs wrote: > On Wed, 2010-01-20 at 04:47 +0100, Andres Freund wrote: > > On Saturday 16 January 2010 12:32:35 Simon Riggs wrote: > > > No. As mentioned upthread, this is not a bug. > > > > Could you also mention in a little bit more detail why not? > > When a cleanup record arrives without a latestRemovedXid we are forced > to assume that the xid could be as late as latestCompletedXid. > Regrettably we aren't certain which of the xids are still there since it > is possible that earlier xids in KnownAssignedXids are actually FATAL > errors that did not write abort records. So we need to conflict with all > current snapshots whose xmin is less than latestCompletedXid to be safe. > This can cause false positives in our assessment of which vxids > conflict. > By using exclusive lock we prevent new snapshots from being taken while > we work out which snapshots to conflict with. This protects those new > snapshots from also being included in our conflict list. > > After the lock is released, we allow snapshots again. It is possible > that we arrive at a snapshot that is identical to one that we just > decided we should conflict with. This a case of false positives, not an > actual problem. > > There are two cases: (1) if we were correct in using latestCompletedXid > then that means that all xids in the snapshot lower than that are FATAL > errors, so not xids that ever commit. We can make no visibility errors > if we allow such xids into the snapshot. (2) if we erred on the side of > caution and in fact the latestRemovedXid should have been earlier than > latestCompletedXid then we conflicted with a snapshot needlessly. Taking > another identical snapshot is OK, because the earlier conflicted > snapshot was a false positive. > > In either case, a snapshot taken after conflict assessment will still be > valid and non-conflicting even if an identical snapshot that existed > before conflict assessment was assessed as conflicting. > > If we allowed concurrent snapshots while we were deciding who to > conflict with we would need to include all concurrent snapshotters in > the conflict list as well. We'd have difficulty in working out exactly > who that was, so it is happier for all concerned if we take an exclusive > lock. > > It also means that users waiting for a snapshot is a good thing, since > it is more likely that they will live longer after having waited. So its > not a bug for us to use exclusive lock and is actually desirable. > > We could reduce false positives by having the master calculate the exact > xmin each time it issues an XLOG_BTREE_DELETE record. That would > introduce more contention since that happens during btree split > operations, so might be counter productive. Wow. Thanks for the extensive explanation! I do understand it correctly that in CancelVirtualTransaction LW_SHARED is taken only so that another transaction can finish during that time? Andres
On Wed, 2010-01-20 at 14:13 +0100, Andres Freund wrote: > I do understand it correctly that in CancelVirtualTransaction > LW_SHARED is > taken only so that another transaction can finish during that time? We're canceling one specific vxid, so no need to block other snapshots from being taken. Read only queries don't take ProcArrayLock when they complete and the Startup process is the only process to record xact completion during recovery. -- Simon Riggs www.2ndQuadrant.com