On 17 June 2013 06:33, David Fetter <da...@fetter.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
