Message-ID: <4E8ED2A7.2040800@gmail.com>
Date: Fri, 07 Oct 2011 12:21:27 +0200
From: Yeb Havinga <yebhavinga@gmail.com>
User-Agent: Mozilla/5.0 (Windows NT 5.1;
	rv:7.0.1) Gecko/20110929 Thunderbird/7.0.1
MIME-Version: 1.0
To: Royce Ausburn <royce.ml@inomial.com>
CC: Pg Hackers <pgsql-hackers@postgresql.org>
Subject: Re: [REVIEW] Patch for cursor calling with named parameters
References: <FCEADDEF-8563-4965-8D4A-E37F2328C42B@inomial.com>
In-Reply-To: <FCEADDEF-8563-4965-8D4A-E37F2328C42B@inomial.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit

On 2011-10-06 16:04, Royce Ausburn wrote:
> Initial Review for patch:
>
> http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php

Hello Royce,

Thank you for your review.

>
> I don't think so.  The new feature accepts opening a cursor with some 
> parameter names not specified:
>
>       open cur1(param3 := 4, 1, param1 := 5);
>
> It seems that if a parameter is not named, its position is used to 
> bind to a variable.  For example, the following fail:
>
> psql:plsqltest.sql:26: ERROR:  cursor "cur1" argument 2 "param2" 
> provided multiple times
> LINE 10:       open cur1(param3 := 4, 1, param2 := 5);
>
> and
>
> psql:plsqltest.sql:26: ERROR:  cursor "cur1" argument 2 "param2" 
> provided multiple times
> LINE 10:       open cur1(param2 := 4, 2, param1 := 5);
>
>
> I think that postgres ought to enforce some consistency here.  Use one 
> way or the other, never both.

This was meant as a feature, but I can remove it.
>
>
> I can also produce some unhelpful errors when I give bad syntax.  For 
> example:
>
> psql:plsqltest.sql:28: ERROR:  cursor "cur1" argument 1 "param1" 
> provided multiple times
> LINE 11:       open cur1( param3 : = 4, 2, param1 := 5);
> (notice the space between the : and =)

Yes, the whole of the expression before the first comma, 'param3 : = 4' 
is not recognized as <parametername> <:= symbol> <expression>, so that 
is taken as the value of the first parameter. This value is parsed after 
all named arguments are read, and hence no meaningful error is given. If 
there was no param1 parameter name at the end, the 'multiple times' 
error would not have caused the processing to stop, and a syntax error 
at the correct : would have been given.

The same reasoning also explains the other 'multiple times' errors you 
could get, by putting a syntax error in some value.
>
> --
>
>       open cur1( param3 := param3 , param2 = 3, param1 := 1 );
>
> psql:plsqltest.sql:29: ERROR:  column "param2" does not exist
> LINE 2: ,param2 = 3
>          ^
> QUERY:  SELECT 1
> ,param2 = 3
> ,param3;
> CONTEXT:  PL/pgSQL function "named_para_test" line 7 at OPEN

This is a valid error, since the parser / SQL will try to evaluate the 
boolean expression param2 = 3, while param2 is not a defined variabele.

Again, thank you very much for your thorough review. I'll update the 
patch so mixing positional and named parameters are removed, add 
documentation, and give syntax errors before an error message indicating 
that positional and named parameters were mixed.

-- 
Yeb Havinga
http://www.mgrid.net/
Mastering Medical Data