Horton

Thread

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

    Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-12-12T19:55:05Z

    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

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

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-12-13T16:22:13Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2011-12-13T17:34:19Z

    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

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

    Yeb Havinga <yebhavinga@gmail.com> — 2011-12-14T10:31:55Z

    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;

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

    Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2011-12-14T13:58:39Z

    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