Re: SKIP LOCKED DATA (work in progress) - Mailing list pgsql-hackers

On 24 July 2014 00:52, Thomas Munro <munro@ip9.org> wrote:
> On 23 July 2014 13:15, David Rowley <dgrowleyml@gmail.com> wrote:
>> I'm also wondering about this block of code in general:
>>
>> if (erm->waitPolicy == RWP_WAIT)
>> wait_policy = LockWaitBlock;
>> else if (erm->waitPolicy == RWP_SKIP )
>> wait_policy = LockWaitSkip;
>> else /* erm->waitPolicy == RWP_NOWAIT */
>> wait_policy = LockWaitError;
>>
>> Just after this code heap_lock_tuple() is called, and if that returns
>> HeapTupleWouldBlock, the code does a goto lnext, which then will repeat that
>> whole block again. I'm wondering why there's 2 enums that are for the same
>> thing? if you just had 1 then you'd not need this block of code at all, you
>> could just pass erm->waitPolicy to heap_lock_tuple().
>
> True.  Somewhere upthread I mentioned the difficulty I had deciding
> how many enumerations were needed, for the various subsystems, ie
> which headers and type they were allowed to share.  Then I put off
> working on this for so long that a nice example came along that showed
> me the way: the lock strength enums LockTupleMode (heapam.h) and
> RowMarkType (plannodes.h).  The wait policy enums LockWaitPolicy
> (heapam.h) and RowWaitPolicy (plannodes.h) mirror them closely, and
> the same type of enumeration translation takes place in nodeLockRows.c
> immediately above the code you pasted.  I don't have any more
> principled argument than monkey-see-monkey-do for that one...

On reflection, I agree that this sucks, and I would like to unify the
three new enums in the current patch (see below for recap) into one
that can be passed between parser, planner, executor and heap access
manager code as I think you are suggesting.  My only question is: in
which header should the enum be defined, that all those modules could
include?

Best regards,
Thomas Munro


Enumeration explosion recap:

* parsenode.h defines enum LockClauseWaitPolicy, which is used in the structs LockClause and RowMarkClause, for use by
theparser code
 

* plannodes.h defines enum RowWaitPolicy, which is used in the structs PlanRowMark and ExecRowMark, for use by the
plannerand executor code (numbers coincide with LockClauseWaitPolicy)
 

* heapam.h defines enum LockWaitPolicy, which is used as a parameter to heap_lock_tuple, for use by heap access code

The parser produces LockClauseWaitPolicy values.  InitPlan
converts these to RowWaitPolicy values in execMain.c.  Then
nodeLockRows.c converts RowWaitPolicy values to LockWaitPolicy
values (by if-then-else) so it can call heap_lock_tuple.

This roughly mirrors what happens to lock strength information.
The unpatched code simply passes a boolean 'nowait' flag around.
An earlier version of my patch passed a pair of booleans around.
Simon's independent patch[1] uses an int in the various node structs
and the heap_lock_tuple function, and in execNode.h it has macros to
give names to the values, and that is included by access/heapm.h.

[1] http://www.postgresql.org/message-id/537610D5.3090405@2ndquadrant.com