I've spent some time reviewing this patch - looks pretty good! I'm not
through yet, but I wanted to post an update. Attached is a new version,
with some modifications I made. Notably:
I added a new struct to hold the per-function executor state - tupdesc,
tuplestore, rowcount and slot - instead of the lists that need to be
kept in sync. This is more readable.
I replaced the CreateTupleDescCopyMany() function with a function called
TupleDescCopyEntry(), which initializes a single attribute like
TupleDescInitEntry(), but copies the information from another tupledesc.
It is used in a loop to construct the composite tupledec in multiple
function or ordinality case. This is more flexible, no need to create
the dummy single-attribute TupleDesc for ordinality anymore, for example.
I refactored the grammar a bit; the way func_table rule returned a one-
or two element list to essentially pass up a flag was an ugly hack.
Below are a couple of more comments:
On 13.09.2013 22:03, Andrew Gierth wrote:
> ***************
> *** 3529,3534 **** static Expr *
> --- 3539,3545 ----
> simplify_function(Oid funcid, Oid result_type, int32 result_typmod,
> Oid result_collid, Oid input_collid, List **args_p,
> bool funcvariadic, bool process_args, bool allow_non_const,
> + FuncExpr *orig_funcexpr,
> eval_const_expressions_context *context)
> {
> List *args = *args_p;
The new argument needs to be explained in the comment above.
> ! <para>
> ! The special table function <literal>UNNEST</literal> may be called with
> ! any number of array parameters, and returns a corresponding number of
> ! columns, as if <literal>UNNEST</literal>
> ! (see <xref linkend="functions-array">) had been called on each parameter
> ! separately and combined using the <literal>TABLE</literal> construct. The
> ! number of result columns is determined by the sum of the arities of the
> ! array element types; that is to say, any array of composite type is
> ! expanded into separate result columns for each field of the type.
> ! Likewise, the number of rows returned is determined by the largest array
> ! parameter, with smaller values padded with NULLs.
> ! </para>
"Arities", really? :-). Please reword that into more plain English.
I'm going to put this aside for a day or two now, but will get back to
it later to finish the review and commit (unless someone beats me to
it). Meanwhile, if you could do something about that comment and manual
paragraph above, and re-review the changes I made, that would be great.
- Heikki
