Skip site navigation (1) Skip section navigation (2)

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

From: Yeb Havinga <yebhavinga(at)gmail(dot)com>
To: Kevin Grittner <Kevin(dot)Grittner(at)wicourts(dot)gov>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: [REVIEW] Patch for cursor calling with named parameters
Date: 2011-12-05 10:06:47
Message-ID: 4EDC97B7.6030508@gmail.com (view raw or flat)
Thread:
Lists: pgsql-hackers
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


Attachment: cursornamedparameter-v5.patch
Description: text/x-patch (24.3 KB)

In response to

Responses

pgsql-hackers by date

Next:From: Heikki LinnakangasDate: 2011-12-05 13:23:45
Subject: Re: Inlining comparators as a performance optimisation
Previous:From: Daniel FarinaDate: 2011-12-05 09:56:19
Subject: Re: Notes on implementing URI syntax for libpq

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group