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

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(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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