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

On 23 July 2013 06:10, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Andrew Gierth <andrew(at)tao11(dot)riddles(dot)org(dot)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
* function appearing 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, without the 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 function scan WITH ORDINALITY */
TupleDesc func_tupdesc;
TupleTableSlot *func_slot;
int64 ordinal;
} FunctionScanState;

Regards,
Dean