Re: WIP: Detecting SSI conflicts before reporting constraint violations

On Sat, Mar 12, 2016 at 1:25 AM, Kevin Grittner <kgrittn(at)gmail(dot)com> wrote:
> On Thu, Mar 10, 2016 at 11:31 PM, Thomas Munro
> <thomas(dot)munro(at)enterprisedb(dot)com> wrote:
>
>> Here's a much simpler version with more comments
>
>> It handles the same set of isolation test specs.
>
> I'm impressed that you found a one-line patch that seems to get us
> 90% of the way to a new guarantee; but I think if we're going to do
> something here it should take us from one clear guarantee to
> another. We really don't have a new guarantee here that is easy to
> express without weasel-words. :-(

+1

> Let me see whether I can figure
> out how to cover that one permutation that is left after this
> one-liner.
>
> In terms of theory, one way to look at this is that an insert of an
> index tuple to a unique index involves a read from that index which
> finds a "gap", which in SSI is normally covered by a predicate lock
> on the appropriate part of the index. (Currently that is done at
> the page level, although hopefully we will eventually enhance that
> to use "next-key gap locking".) Treating the index tuple insertion
> as having an implied read of that gap is entirely justified and
> proper -- internally the read actually does happen.

Right, that is what I was trying to achieve in earlier versions
immediately after _bt_check_unique returns without error, with
something like this:

+ /*
+ * By determining that there is no duplicate key, we have read an index
+ * gap, so we predicate lock it.
+ */
+ PredicateLockPage(rel, buf, GetTransactionSnapshot());

But since a conflict-in check immediately follows (because we insert),
wouldn't that be redundant (ie self-created SIREAD locks are dropped
at that point)?

Next I thought that a conflict-out check on the heap tuple might be
necessary to detect a problem here. Back in the v2 patch I was making
an explicit call to CheckForSerializationConflictOut, but I now see
that a plain old heap_fetch would do that. I was also using the wrong
htid because I missed that the CIC special case code reassigns htid.
See the attached experimental patch which applies on top of the v4 one
liner patch, adding a call to heap_fetch the conflicting tuple. This
way, read-write-unique-4.out permutation "r1 w1 w2 c1 c2" now raises a
40001 after c1 completes and w2 continues, and no other isolation test
result changes. So still no cigar for "r2 w1 w2 c1 c2". Stepping
through CheckForSerializationConflictOut when it is called before w2
reports a UCV, I see that it returns here:

if (RWConflictExists(MySerializableXact, sxact))
{
/* We don't want duplicate conflict records in the list. */
LWLockRelease(SerializableXactHashLock);
return;
}

So the RW conflict (ie one edge) is detected (and already had been?),
but not a dangerous structure (there are not two consecutive edges in
the graph? We know that we have read something written by an
overlapping transaction, but not that it has read something
[hypothetically] written by us?). I will have another look at this in
a few days but for now I need to do some other things, so I'm posting
these observations in case they are in some way helpful...

--
Thomas Munro
http://www.enterprisedb.com