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. [...]
I tried getting rid of the offending if-then-else enum conversion code
and replaced it with a simple assignment -- please see attached. I
also added compile time assertions that the enum values line up to
make that work, and are correctly ordered for use in that 'Max'
expression. Please let me know if you think this is an improvement or
an abomination.
I couldn't find an existing reasonable place to share a single wait
policy enumeration between parser/planner/executor and the heap access
module, and I get the feeling that it would be unacceptable to
introduce one.
I suppose that the LockClauseWaitPolicy and RowWaitPolicy could at
least be merged into a single enum defined in nodes.h following the
example of CmdType, which is also used by both parsenodes.h and
plannnode.h, but do I detect a tiny hint of reluctance in its comment,
"so put it here..."?
(The attached patch also has a couple of trivial typo fixes in
documentation and comments).
Best regards,
Thomas Munro