Thread: Small locking bugs in hs

Small locking bugs in hs

From
Andres Freund
Date:
Hi,

While unlikely to cause issues two new LWLockAcquire calls use the wrong 
locking mode.
Patch attached.

Andres

Re: Small locking bugs in hs

From
Simon Riggs
Date:
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



Re: Small locking bugs in hs

From
Andres Freund
Date:
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


Re: Small locking bugs in hs

From
Andres Freund
Date:
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


Re: Small locking bugs in hs

From
Simon Riggs
Date:
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



Re: Small locking bugs in hs

From
Robert Haas
Date:
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


Re: Small locking bugs in hs

From
Simon Riggs
Date:
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



Re: Small locking bugs in hs

From
Andres Freund
Date:
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


Re: Small locking bugs in hs

From
Simon Riggs
Date:
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



Re: Small locking bugs in hs

From
Andres Freund
Date:
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


Re: Small locking bugs in hs

From
Simon Riggs
Date:
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