The patch is in context format, includes docs and additional
regression tests, applies cleanly, passes "world" regression tests.
The parser changes don't cause any "backing up".
Discussion on the list seems to have reached a consensus that this
patch implements a useful feature. A previous version was reviewed.
This version attempts to respond to points previously raised.
Overall, it looked good, but there were a few things I would like Yeb
to address before mark it is marked ready for committer. I don't
think any of these will take long.
(1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what
changed.(b) There's one point where "cursors" should be possessive "cursor's". (c) I think it would be less
confusingto be consistent about mentioning "positional" and "named" in the same order each time. Mixing it up
likethat is harder to read, at least for me.
(2) The error for mixing positional and named parameters should be
moved up. That seems more important than "too many arguments" or
"provided multiple times" if both are true.
(3) The "-- END ADDITIONAL TESTS" comment shouldn't be added to the
regression tests.
The way this parses out the named parameters and then builds the
positional list, with some code from read_sql_construct() repeated in
read_cursor_args() seems a little bit clumsy to me, but I couldn't
think of a better way to do it.
-Kevin