Re: SKIP LOCKED DATA (work in progress)

On 17 May 2014 05:02, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> On 05/17/2014 05:24 AM, Thomas Munro wrote:
> > I noticed that in applyLockingClause, Simon has "rc->waitMode |=
> > waitMode". Is that right? The values are 0, 1, and
2, but if you had
> > both NOWAIT and SKIP LOCKED somewhere in your query you could up with
> > rc->waitMode == 3 unless I'm mistaken.
>
> I do not think that |= is correct there.
>
> It may be that no case can arise where you get the bogus value, but
> since in all other places the values are tested for equalty not as bit
> fields ( waitMode == NOWAIT not waitMode & NOWAIT ) it doesn't make
> sense to |= it there.

There are indeed ways (subselects etc) to have more than one
locking clause applying to the same table, which are supposed to
be merged into the appropriate wait policy by that line.

> ? In my patch I have code that
> > would give precedence to NOWAIT, though looking at it again it could be
> > simpler.
>
> I agree; if NOWAIT is present anywhere it should be preferred to
> SKIP_LOCKED, as it's OK to apply NOWAIT where
SKIP LOCKED appears, but
> possibly semantically incorrect to apply SKIP LOCKED where only NOWAIT
> was expected.

In the new v5 patch (attached) I used the same approach as
the "lock strength" support, that is, relying on the values of
the enumeration being arranged so that the enumerators that
should take precendence have higher numerical values so I could
just write:

rc->strength = Max(rc->strength, strength);

+ rc->waitPolicy = Max(rc->waitPolicy, waitPolicy);

> > (One reviewer pointed out, that it should really be a single
> > unified enum. In fact I have been a bit unsure about what scope such an
> > enumeration should have in the application -- could it even be used in
> > parser code? I tried to follow existing examples which is why I used
> > #define macros in gram.y).
>
> Not sure there.

I removed those macros and went for an enumeration -- see below.

> > From a bikeshed colour point of view:
> > * I used SKIP LOCKED DATA like DB2, and Simon used SKIP LOCKED like
> > Oracle, and I guess shorter is sweeter
>
> We have a long tradition of trying to allow noise keywords where it's
> harmless.
>
> So the clause should probably be
>
> SKIP LOCKED [DATA]

Ok, done in the v5 patch.

> I have some isolation tester and regression tests that are still to
follow.

Great, I am looking forward to seeing those. I have added an
isolation test for NOWAIT, and I am planning to design some tests
that will test the various affected branched of
heap_lock_tuple (eg multi-transaction ID etc), and test
interaction with various other features (various lock stregths,
UPDATEs, combinations of SKIP LOCKED DATA, NOWAIT and default
policies etc).

> > * I had noWait and skipLocked travelling separately in some places,
> > Simon had a single parameter, which is much better
>
> Yes, I strongly prefer that.

I agree in principal but couldn't decide *how many* enumerations
to use. In v5 I have added two more:

* one called LockClauseWaitPolicy defined in parsenodes.h that is
used in LockClause and RowMarkCause

* another called RowWaitPolicy defined in plannodes.h that is
used in PlanRowMark and ExecRowMark

The parser produces LockClauseWaitPolicy values, which InitPlan
converts to RowWaitPolicy values by simple assignment (the values
are the same, which obviously wouldn't work if this were compiled
as C++). Then ExecLockRows converts those into LockWaitPolicy
values when calling heal_lock_tuple.

Obviously there is an opportunity to unify all three, or at least
LockClauseWaitPolicy (parser) and RowWaitPolicy (plan and
execution), but I didn't know how appropriate that would be, and
followed the new lock strength feature for inspiration.

One difference between my patch and Simon's is that mine
introduces a new return value for heap_lock_type called
HeapTupleWouldBlock, whereas his returns the existin
HeapTupleBeingUpdated and then handles it differently in
ExecLockRows if in SKIP LOCKED mode.

Best regards,
Thomas Munro