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

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@gmail.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.Grittner@wicourts.gov> wrote:

Yeb Havinga <yebhavinga@gmail.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@wicourts.gov> wrote:

Yeb Havinga <yebhavinga@gmail.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@wicourts.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@wicourts.gov> wrote:

Yeb Havinga<yebhavinga@gmail.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@wicourts.gov> wrote:

Yeb Havinga<yebhavinga@gmail.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

Yeb Havinga wrote:

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

This version addresses all of my concerns. It applies cleanly and
compiles without warning against current HEAD and performs as
advertised. I'm marking it Ready for Committer.

-Kevin

On 12.12.2011 21:55, Kevin Grittner wrote:

Yeb Havinga wrote:

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

This version addresses all of my concerns. It applies cleanly and
compiles without warning against current HEAD and performs as
advertised. I'm marking it Ready for Committer.

This failed:

postgres=# do $$
declare
foocur CURSOR ("insane /* arg" int4) IS SELECT generate_series(1,
"insane /* arg");
begin
OPEN foocur("insane /* arg" := 10);
end;
$$;
ERROR: unterminated /* comment at or near "/* insane /* arg := */ 10;"
LINE 1: SELECT /* insane /* arg := */ 10;
^
QUERY: SELECT /* insane /* arg := */ 10;
CONTEXT: PL/pgSQL function "inline_code_block" line 5 at OPEN

I don't have much sympathy for anyone who uses argument names like that,
but it nevertheless ought to not fail. A simple way to fix that is to
constuct the query as: "value AS argname", instead of "/* argname := */
value". Then you can use the existing quote_identifier() function to do
the necessary quoting.

I replaced the plpgsql_isidentassign() function with a more generic
plpgsql_peek2() function, which allows you to peek ahead two tokens in
the input stream, without eating them. It's implemented using the
pushdown stack like plpgsql_isidentassign() was, but the division of
labor between pl_scanner.c and gram.y seems more clear this way. I'm
still not 100% happy with it. plpgsql_peek2() behaves differently from
plpgsql_yylex(), in that it doesn't perform variable or unreserved
keyword lookup. It could do that, but it would be quite pointless since
the only current caller doesn't want variable or unreserved keyword
lookup, so it would just have to work harder to undo them.

Attached is a patch with those changes. I also I removed a few of the
syntax error regression tests, that seemed excessive, plus some general
naming and comment fiddling. I'll apply this tomorrow, if it still looks
good to me after sleeping on it.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Attached is a patch with those changes. I also I removed a few of the
syntax error regression tests, that seemed excessive, plus some general
naming and comment fiddling. I'll apply this tomorrow, if it still looks
good to me after sleeping on it.

The code looks reasonably clean now, although I noted one comment
thinko:

+ * bool: trim trailing whitespace

ITYM

+ * trim: trim trailing whitespace

However, I'm still concerned about whether this approach gives
reasonable error messages in cases where the error would be
detected during parse analysis of the rearranged statement.
The regression test examples don't cover such cases, and I'm
too busy right now to apply the patch and check for myself.
What happens for example if a named parameter's value contains
a misspelled variable reference, or a type conflict?

regards, tom lane

On 2011-12-13 18:34, Tom Lane wrote:

Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:

Attached is a patch with those changes. I also I removed a few of the
syntax error regression tests, that seemed excessive, plus some general
naming and comment fiddling. I'll apply this tomorrow, if it still looks
good to me after sleeping on it.

However, I'm still concerned about whether this approach gives
reasonable error messages in cases where the error would be
detected during parse analysis of the rearranged statement.
The regression test examples don't cover such cases, and I'm
too busy right now to apply the patch and check for myself.
What happens for example if a named parameter's value contains
a misspelled variable reference, or a type conflict?

I tested this and seems to be ok:

regression=# select namedparmcursor_test1(20000, 20000) as "Should be
false",
namedparmcursor_test1(20, 20) as "Should be true";
ERROR: column "yy" does not exist
LINE 1: SELECT x AS param1, yy AS param12;

regression=# select namedparmcursor_test1(20000, 20000) as "Should be
false",
namedparmcursor_test1(20, 20) as "Should be true";
ERROR: invalid input syntax for integer: "2011-11-29 19:26:10.029084"
CONTEXT: PL/pgSQL function "namedparmcursor_test1" line 8 at OPEN

regards,
Yeb Havinga

last error was created with

create or replace function namedparmcursor_test1(int, int) returns
boolean as $$
declare
c1 cursor (param1 int, param12 int) for select * from rc_test where
a > param1 and b > param12;
y int := 10;
x timestamp := now();
nonsense record;
begin
open c1(param12 := $1, param1 := x);
end
$$ language plpgsql;

On 14.12.2011 12:31, Yeb Havinga wrote:

On 2011-12-13 18:34, Tom Lane wrote:

Heikki Linnakangas<heikki.linnakangas@enterprisedb.com> writes:

Attached is a patch with those changes. I also I removed a few of the
syntax error regression tests, that seemed excessive, plus some general
naming and comment fiddling. I'll apply this tomorrow, if it still looks
good to me after sleeping on it.

However, I'm still concerned about whether this approach gives
reasonable error messages in cases where the error would be
detected during parse analysis of the rearranged statement.
The regression test examples don't cover such cases, and I'm
too busy right now to apply the patch and check for myself.
What happens for example if a named parameter's value contains
a misspelled variable reference, or a type conflict?

I tested this and seems to be ok:

regression=# select namedparmcursor_test1(20000, 20000) as "Should be
false",
namedparmcursor_test1(20, 20) as "Should be true";
ERROR: column "yy" does not exist
LINE 1: SELECT x AS param1, yy AS param12;

For the record, the caret pointing to the position is there, too. As in:

regression=# do $$
declare
c1 cursor (param1 int, param2 int) for select 123;
begin
open c1(param2 := xxx, param1 := 123);
end;
$$;
ERROR: column "xxx" does not exist
LINE 1: SELECT 123 AS param1, xxx AS param2;
^
QUERY: SELECT 123 AS param1, xxx AS param2;
CONTEXT: PL/pgSQL function "inline_code_block" line 5 at OPEN

I think that's good enough. It would be even better if we could print
the original OPEN statement as the context, as in:

ERROR: column "xxx" does not exist
LINE 4: OPEN c1(param2 := xxx, param1 := 123);
^

but it didn't look quite like that before the patch either, and isn't
specific to this patch but more of a general usability issue in PL/pgSQL.

Committed.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com