Dan Ports wrote:
> On Wed, Jun 08, 2011 at 09:17:04PM -0500, Kevin Grittner wrote:
>> A patch is attached which just covers the predicate lock
>> acquisition, where a snapshot is available without too much pain.
>> There are two functions which acquire predicate locks where a
>> snapshot was not readily available: _bt_search() and
>> _bt_get_endpoint(). Not only was it not clear how to get a
>> snapshot in, it was not entirely clear from reading the code that
>> we need to acquire predicate locks here. Now, I suspect that we
>> probably do, because I spent many long hours stepping through gdb
>> to pick the spots where they are, but that was about a year ago
>> and my memory of the details has faded.
>
> For _bt_search(), the lock calls should move to _bt_first() where
> the ScanDesc is available. This also keeps us from trying to take
> locks during _bt_pagedel(), which is only called during vacuum and
> recovery.
Sounds reasonable, but why did you pass the snapshot to the
PredicateLockPage() call but not the PredicateLockRelation() call?
Oversight?
> The call in _bt_get_endpoint() seems unnecessary, because after it
> returns, _bt_endpoint() takes the same lock. The only other callers
> of _bt_get_endpoint() are _bt_pagedel() and _bt_insert_parent(),
> neither of which should take predicate locks.
That also sounds reasonable.
> I've updated the patch, attached.
I've confirmed that it passes the usual regression tests (including
isolation tests and the normal regression tests at serializable).
I'll take a closer look once I wake up and get the caffeine going.
Thanks for following up on this!
-Kevin