Initial Review for patch:
http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php
Submission review
The patch is in context diff format and applies cleanly to the git master. The
patch includes an update to regression tests. The regression tests pass. The
patch does not include updates to the documentation reflecting the new
functionality.
Usability review
The patch adds a means of specifying named cursor parameter arguments in
pg/plsql.
The syntax is straight forward:
cur1 CURSOR (param1 int, param2 int, param3 int) for select …;
…
open cur1(param3 := 4, param2 := 1, param1 := 5);
The old syntax continues to work:
cur1 CURSOR (param1 int, param2 int, param3 int) for select …;
…
open cur1(5, 1, 3);
• Does the patch actually implement that?
Yes, the feature works as advertised.
• Do we want that?
I very rarely use pg/plsql, so I won't speak to its utility. However there has
been some discussion about the idea:
http://archives.postgresql.org/pgsql-hackers/2010-09/msg01440.php
• Do we already have it?
Not AFAIK
• Does it follow SQL spec, or the community-agreed behavior?
There's some discussion about the syntax ( := or => ), I'm not sure there's a
consensus yet.
• Does it include pg_dump support (if applicable)?
Yes -- pgplsql functions using named parameters are correctly dumped.
• Are there dangers?
Potentially. See below.
• Have all the bases been covered?
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.
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 =)
--
psql:plsqltest.sql:28: ERROR: cursor "cur1" argument 1 "param1" provided
multiple times
LINE 11: open cur1( param3 => 4, 2, param1 := 5);
(Wrong assignment operator)
--
psql:plsqltest.sql:27: ERROR: cursor "cur1" argument 3 "param3" provided
multiple times
LINE 10: open cur1( 1 , param3 := 2, param2 = 3 );
(Wrong assignment operator)
--
psql:plsqltest.sql:27: ERROR: cursor "cur1" argument 3 "param3" provided
multiple times
LINE 10: ... open cur1( param3 = param3 , param3 := 2, param2 = 3 );
--
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
I haven't been able to make something syntactically incorrect that doesn't
produce an error, even if the error isn't spot on. I haven't been able to make
it crash, and when the syntax is correct, it has always produced correct
results.
Performance review
I've done some limited performance testing and I can't really see a difference
between the unpatched and patched master.
• Does it follow the project coding guidelines?
I believe so, but someone more familiar with them will probably spot violations
better than me =)
• Are there portability issues?
I wouldn't know -- I don't have any experience with C portability.
• Will it work on Windows/BSD etc?
Tested under OS X, so BSD is presumably okay. No idea about other unixes nor
windows.
• Are the comments sufficient and accurate?
I'm happy enough with them.
• Does it do what it says, correctly?
Yes, excepting my comments above.
• Does it produce compiler warnings?
Yes:
In file included from gram.y:12962:
scan.c: In function ‘yy_try_NUL_trans’:
scan.c:16243: warning: unused variable ‘yyg’
But this was not added by this patch -- it's also in the unpatched master.
• Can you make it crash?
No.
--Royce
