Re: SSI patch version 14

Kevin Grittner <kgrittn@wicourts.gov> wrote:
> "Kevin Grittner"  wrote:
>> Jeff Davis wrote:
>>  
>>> What does PredicateLockTuple do that needs a share lock? Does a
>>> pin suffice?
>> 
>> If one process is reading a tuple and another is writing it
>> (e.g., UPDATE or DELETE) the concern is that we need to be able
>> to guarantee that either the predicate lock appears in time for
>> the writer to see it on the call to
>> CheckForSerializableConflictIn or the reader sees the MVCC
>> changes in CheckForSerializableConflictOut. It's OK if the
>> conflict is detected both ways, but if it's missed both ways then
>> we could have a false negative, allowing anomalies to slip
>> through. It didn't seem to me on review that acquiring the
>> predicate lock after releasing the shared buffer lock (and just
>> having the pin) would be enough to ensure that a write which
>> followed that would see the predicate lock.
>> 
>> reader has pin and shared lock
>> writer increments pin count
>> reader releases shared lock
>> writer acquires exclusive lock
>> writer checks predicate lock and fails to see one
>> reader adds predicate lock
>> we have a problem
>  
> Hmmm...  Or do we?  If both sides were careful to record what
> they're doing before checking for a conflict, the pin might be
> enough. I'll check for that.  In at least one of those moves I was
> moving the predicate lock acquisition from after the conflict
> check to before, but maybe I didn't need to move it quite so
> far....
Some of these calls are placed where there is no reasonable choice
but to do them while holding a buffer lock.  There are some which
*might* be able to be moved out, but I'm inclined to say that should
be on a list of possible performance improvements in future
releases.  My concern is that the code paths are complicated enough
to make it hard to confidently desk-check such changes, we don't yet
have a good way to test whether such a change is introducing a race
condition.  The src/test/isolation tests are good at proving the
fundamental correctness of the logic in predicate.c, and the DBT-2
stress tests Dan ran are good at flushing out race conditions within
predicate.c code; but we don't have a test which is good at pointing
out race conditions based on *placement of the calls* to predicate.c
from other code.
Until we have such tests, I think we should be cautious about
risking possible hard-to-reproduce correctness violations to shave a
few nanoseconds off the time a shared buffer lock is held. 
Particularly since we're so close to the end of the release cycle.
-Kevin