Review: UNNEST (and other functions) WITH ORDINALITY

On 17 June 2013 06:33, David Fetter <david(at)fetter(dot)org> wrote:
>> Next revision of the patch, now with more stability. Thanks, Andrew!
>
> Rebased vs. git master.
>

Here's my review of the WITH ORDINALITY patch.

Overall I think that the patch is in good shape, and I think that this
will be a very useful new feature, so I'm keen to see this committed.

All the basic stuff is OK --- the patch applies cleanly, compiles with
no warnings, and has appropriate docs and new regression tests which
pass. I also tested it fairly thoroughly myself, and I couldn't break
it. Everything worked as I expected, and it works nicely with LATERAL.

I have a few minor comments that should be considered before passing
it on to a committer:

1). In func.sgml, the new doc example "unnest WITH ORDINALITY" is
mislablled, since it it's not actually an example of unnest(). Also it
doesn't really belong in that code example block, which is about
generate_subscripts(). I think that there should probably be a new
sub-section starting at that point for WITH ORDINALITY. Perhaps it
should start with a brief paragraph explaining how WITH ORDINALITY can
be applied to functions in the FROM clause of a query.

[Actually it appears that WITH ORDINALITY works with non-SRF's too,
but that's less useful, so I think that the SRF section is probably
still the right place to document this]

It might also be worth mentioning here that currently WITH ORDINALITY
is not supported for functions that return records.

In the code example itself, the prompt should be trimmed down to match
the previous examples.

2). In the SELECT docs, where function_name is documented, I would be
tempted to start a new paragraph for the sentence starting "If the
function has been defined as returning the record data type...", since
that's really a separate syntax. I think that should also make mention
of the fact that WITH ORDINALITY is not currently supported in that
case.

3). I think it would be good to have a more meaningful default name
for the new ordinality column, rather than "?column?". In many cases
the user might then choose to not bother giving it an alias. It could
simply be called ordinality by default, since that's non-reserved.

4). In gram.y, WITH_ORDINALITY should not be listed as an ordinary
keyword, but instead should be listed as a token below that (just
before WITH_TIME).

5). In plannodes.h, FunctionScan's new field should probably have a comment.

6). In parsenodes.h, the field added to RangeTblEntry is only valid
for function RTEs, so it should be moved to that group of fields and
renamed appropriately (unless you're expecting to extend it to other
RTE kinds in the future?). Logically then, the new check for
ordinality in inline_set_returning_function() should be moved so that
it is after the check that the RTE actually a function RTE, and in
addRangeTableEntryForFunction() the RTE's ordinality field should be
set at the start along with the other function-related fields.

7). In execnodes.h, the new fields added to FunctionScanState should
be documented in the comment block above.

Overall, as I said, I really like this feature, and I think it's not
far from being commitable.

Regards,
Dean