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.
? 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.
> (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.
> 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]
in much the same way we have
BEGIN [ WORK | TRANSACTION ] ...
There won't be any ambiguity there.
> * I used the term wait_policy and an enum, Simon used waitMode and an int
I prefer an enum and intended to change Simon's patch but didn't have
the time.
I have some isolation tester and regression tests that are still to follow.
> * I had noWait and skipLocked travelling separately in some places,
> Simon had a single parameter, which is much better
Yes, I strongly prefer that.
-- Craig Ringer http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
