[REVIEW] Patch for cursor calling with named parameters

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