Re: Review: UNNEST (and other functions) WITH ORDINALITY

On 23 July 2013 06:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Andrew Gierth <andrew@tao11.riddles.org.uk> writes:
>> I must admit to finding all of this criticism of unread code a bit
>> bizarre.
>
> If you yourself are still finding bugs in the code as of this afternoon,
> onlookers could be forgiven for doubting that the code is quite as
> readable or ready to commit as all that.
>

I had another look at this --- the bug fix looks reasonable, and
includes a sensible set of additional regression tests.

This was not a bug that implies anything fundamentally wrong with the
patch. Rather it was just a fairly trivial easy-to-overlook bug of
omission --- I certainly overlooked it in my previous reviews (sorry)
and at least 3 more experienced hackers than me overlooked it during
detailed code inspection.

I don't think that really reflects negatively on the quality of the
patch, or the approach taken, which I still think is good. That's also
backed up by the fact that Greg isn't able to find much he wants to
change.

I also don't see much that needs changing in the patch, except
possibly in the area where Greg expressed concerns over the comments
and code clarity. I had similar concerns during my inital review, so I
tend to agree that perhaps it's worth adding a separate boolean
has_ordinality flag to FunctionScanState just to improve code
readability. FWIW, I would probably have extended FunctionScanState
like so:

/* ----------------*   FunctionScanState information**      Function nodes are used to scan the results of a*
functionappearing in FROM (typically a function returning set).**      eflags              node's capability flags*
tupdesc             node's expected return tuple description*      tuplestorestate     private state of tuplestore.c*
  funcexpr            state for function expression being evaluated*      has_ordinality      function scan WITH
ORDINALITY?*     func_tupdesc        for WITH ORDINALITY, the raw function tuple*                          description,
withoutthe added ordinality column*      func_slot           for WITH ORDINALITY, a slot for the raw function*
               return tuples*      ordinal             for WITH ORDINALITY, the ordinality of the return*
          tuple* ----------------*/
 
typedef struct FunctionScanState
{   ScanState   ss;             /* its first field is NodeTag */   int         eflags;   TupleDesc   tupdesc;
Tuplestorestate*tuplestorestate;   ExprState  *funcexpr;   bool        has_ordinality;   /* these fields are used for a
functionscan WITH ORDINALITY */   TupleDesc   func_tupdesc;   TupleTableSlot *func_slot;   int64       ordinal;
 
} FunctionScanState;


Regards,
Dean