On 23 July 2013 06:10, Tom Lane <t...@sss.pgh.pa.us> wrote: > Andrew Gierth <and...@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 * 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers