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
