Re: [REVIEW] Patch for cursor calling with named parameters

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 confusing to be consistent about
mentioning "positional" and "named" in the same order each
time. Mixing it up like that 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

Kevin,

Thank you for your review!

On 2011-12-03 21:53, Kevin Grittner wrote:
>
> (1) Doc changes:
>
> (a) These contain some unnecessary whitespace changes which make it
> harder to see exactly what changed.

This is probably caused because I used emacs's fill-paragraph (alt+q)
command, after some changes. If this is against policy, I could change
the additions in such a way as to cause minor differences, however I
believe that the benefit of that ends immediately after review.

> (b) There's one point where "cursors" should be possessive
> "cursor's".
>
> (c) I think it would be less confusing to be consistent about
> mentioning "positional" and "named" in the same order each
> time. Mixing it up like that is harder to read, at least for
> me.

Good point, both changed.

> (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.

I moved the error up, though I personally tend to believe it doesn't
even need to be an error. There is no technical reason not to allow it.
All the user needs to do is make sure that the combination of named
parameters and the positional ones together are complete and not
overlapping. This is also in line with calling functions, where mixing
named and positional is allowed, as long as the positional arguments are
first. Personally I have no opinion what is best, include the feature or
give an error, and I removed the possibility during the previous commitfest.

> (3) The "-- END ADDITIONAL TESTS" comment shouldn't be added to the
> regression tests.

This is removed, also the -- START ADDITIONAL TESTS, though I kept the
tests between those to comments.

> 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.

Same here.

The new patch is attached.

regards
Yeb Havinga

Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:
> On 2011-12-03 21:53, Kevin Grittner wrote:

>> (1) Doc changes:
>>
>> (a) These contain some unnecessary whitespace changes which
>> make it harder to see exactly what changed.
>
> This is probably caused because I used emacs's fill-paragraph
> (alt+q) command, after some changes. If this is against policy, I
> could change the additions in such a way as to cause minor
> differences, however I believe that the benefit of that ends
> immediately after review.

I don't know whether it's a hard policy, but people usually minimize
whitespace changes in such situations, to make it easier for the
reviewer and committer to tell what really changed. The committer
can always reformat after looking that over, if they feel that's
useful. The issue is small enough here that I'm not inclined to
consider it a blocker for sending to the committer.

>> (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.
>
> I moved the error up, though I personally tend to believe it
> doesn't even need to be an error. There is no technical reason not
> to allow it. All the user needs to do is make sure that the
> combination of named parameters and the positional ones together
> are complete and not overlapping. This is also in line with
> calling functions, where mixing named and positional is allowed,
> as long as the positional arguments are first. Personally I have
> no opinion what is best, include the feature or give an error, and
> I removed the possibility during the previous commitfest.

If there's no technical reason not to allow them to be mixed, I
would tend to favor consistency with parameter calling rules. Doing
otherwise seems like to result in confusion and "bug" reports.

How do others feel?

-Kevin

"Kevin Grittner" <Kevin(dot)Grittner(at)wicourts(dot)gov> wrote:
> Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:

>> I personally tend to believe it doesn't even need to be an error.
>> There is no technical reason not to allow it. All the user needs
>> to do is make sure that the combination of named parameters and
>> the positional ones together are complete and not overlapping.
>> This is also in line with calling functions, where mixing named
>> and positional is allowed, as long as the positional arguments
>> are first. Personally I have no opinion what is best, include the
>> feature or give an error, and I removed the possibility during
>> the previous commitfest.
>
> If there's no technical reason not to allow them to be mixed, I
> would tend to favor consistency with parameter calling rules.
> Doing otherwise seems like to result in confusion and "bug"
> reports.

Er, that was supposed to read "I would tend to favor consistency
with function calling rules." As stated here:

http://www.postgresql.org/docs/9.1/interactive/sql-syntax-calling-funcs.html

| PostgreSQL also supports mixed notation, which combines positional
| and named notation. In this case, positional parameters are
| written first and named parameters appear after them.

> How do others feel?

If there are no objections, I suggest that Yeb implement the mixed
notation for cursor parameters.

-Kevin

Kevin Grittner <kgrittn(at)wicourts(dot)gov> wrote:
> Yeb Havinga <yebhavinga(at)gmail(dot)com> wrote:

>> I personally tend to believe it doesn't even need to be an error.
>> There is no technical reason not to allow it. All the user needs
>> to do is make sure that the combination of named parameters and
>> the positional ones together are complete and not overlapping.

> If there are no objections, I suggest that Yeb implement the mixed
> notation for cursor parameters.

Hearing no objections -- Yeb, are you OK with doing this, and do you
feel this is doable for this CF?

-Kevin

On 2011-12-06 17:58, Kevin Grittner wrote:
> Kevin Grittner<kgrittn(at)wicourts(dot)gov> wrote:
>> If there are no objections, I suggest that Yeb implement the mixed
>> notation for cursor parameters.
>
> Hearing no objections -- Yeb, are you OK with doing this, and do you
> feel this is doable for this CF?

It is not a large change, I'll be able to do it tomorrow if nothing
unexpected happens, or monday otherwise.

regards,
Yeb

On 2011-12-06 17:58, Kevin Grittner wrote:
> Kevin Grittner<kgrittn(at)wicourts(dot)gov> wrote:
>> Yeb Havinga<yebhavinga(at)gmail(dot)com> wrote:
>
>>> I personally tend to believe it doesn't even need to be an error.
>>> There is no technical reason not to allow it. All the user needs
>>> to do is make sure that the combination of named parameters and
>>> the positional ones together are complete and not overlapping.
>
>> If there are no objections, I suggest that Yeb implement the mixed
>> notation for cursor parameters.
>
> Hearing no objections -- Yeb, are you OK with doing this, and do you
> feel this is doable for this CF?

Attach is v6 of the patch, allowing mixed mode and with new test cases
in the regression tests. One difference with calling functions remains:
it is allowed to place positional arguments after named parameters.
Including that would add code, but nothing would be gained.

regards,
Yeb Havinga

On 2011-12-11 16:26, Yeb Havinga wrote:
> On 2011-12-06 17:58, Kevin Grittner wrote:
>> Kevin Grittner<kgrittn(at)wicourts(dot)gov> wrote:
>>> Yeb Havinga<yebhavinga(at)gmail(dot)com> wrote:
>>
>>>> I personally tend to believe it doesn't even need to be an error.
>>>> There is no technical reason not to allow it. All the user needs
>>>> to do is make sure that the combination of named parameters and
>>>> the positional ones together are complete and not overlapping.
>>
>>> If there are no objections, I suggest that Yeb implement the mixed
>>> notation for cursor parameters.
>>
>> Hearing no objections -- Yeb, are you OK with doing this, and do you
>> feel this is doable for this CF?
>
> Attach is v6 of the patch, allowing mixed mode and with new test cases
> in the regression tests. One difference with calling functions
> remains: it is allowed to place positional arguments after named
> parameters. Including that would add code, but nothing would be gained.

Forgot to copy regression output to expected - attached v7 fixes that.

-- Yeb