Received: from maia.hub.org (maia-3.hub.org [200.46.204.243]) by mail.postgresql.org (Postfix) with ESMTP id D623BB5DC0F for ; Thu, 6 Oct 2011 11:05:24 -0300 (ADT) Received: from mail.postgresql.org ([200.46.204.86]) by maia.hub.org (mx1.hub.org [200.46.204.243]) (amavisd-maia, port 10024) with ESMTP id 30286-01-4 for ; Thu, 6 Oct 2011 14:05:17 +0000 (UTC) X-Greylist: from auto-whitelisted by SQLgrey-1.8.0-rc2 Received: from ix.inomial.com (mail.inomial.net [202.62.159.215]) by mail.postgresql.org (Postfix) with ESMTP id DA27BB5DD3B for ; Thu, 6 Oct 2011 11:04:59 -0300 (ADT) Received: from [192.168.1.4] (ip-143.29.92.203.VOCUS.net.au [203.92.29.143]) by ix.inomial.com (Postfix) with ESMTPA id 7C47A2C1791; Fri, 7 Oct 2011 01:04:56 +1100 (EST) From: Royce Ausburn Content-Type: multipart/alternative; boundary="Apple-Mail=_147B66CB-C081-476C-8B4A-C6578BE2CED1" Subject: [REVIEW] Patch for cursor calling with named parameters Date: Fri, 7 Oct 2011 01:04:55 +1100 Message-Id: Cc: yebhavinga@gmail.com To: Pg Hackers Mime-Version: 1.0 (Apple Message framework v1244.3) X-Mailer: Apple Mail (2.1244.3) X-Virus-Scanned: Maia Mailguard 1.0.1 X-Spam-Status: No, hits=-1.899 tagged_above=-5 required=5 tests=BAYES_00=-1.9, HTML_MESSAGE=0.001 X-Spam-Level: X-Archive-Number: 201110/283 X-Sequence-Number: 196089 --Apple-Mail=_147B66CB-C081-476C-8B4A-C6578BE2CED1 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=windows-1252 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. =20 The syntax is straight forward: cur1 CURSOR (param1 int, param2 int, param3 int) for select =85; =85 open cur1(param3 :=3D 4, param2 :=3D 1, param1 :=3D 5); The old syntax continues to work: cur1 CURSOR (param1 int, param2 int, param3 int) for select =85; =85 open cur1(5, 1, 3); =95 Does the patch actually implement that? Yes, the feature works as advertised. =95 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 =95 Do we already have it? Not AFAIK =95 Does it follow SQL spec, or the community-agreed behavior? There's some discussion about the syntax ( :=3D or =3D> ), I'm not sure = there's a consensus yet. =95 Does it include pg_dump support (if applicable)? Yes -- pgplsql functions using named parameters are correctly dumped. =95 Are there dangers? Potentially. See below. =95 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 :=3D 4, 1, param1 :=3D 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 :=3D 4, 1, param2 :=3D 5); and psql:plsqltest.sql:26: ERROR: cursor "cur1" argument 2 "param2" = provided multiple times LINE 10: open cur1(param2 :=3D 4, 2, param1 :=3D 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 : =3D 4, 2, param1 :=3D 5);=20 (notice the space between the : and =3D) -- psql:plsqltest.sql:28: ERROR: cursor "cur1" argument 1 "param1" = provided multiple times LINE 11: open cur1( param3 =3D> 4, 2, param1 :=3D 5);=20 (Wrong assignment operator) -- psql:plsqltest.sql:27: ERROR: cursor "cur1" argument 3 "param3" = provided multiple times LINE 10: open cur1( 1 , param3 :=3D 2, param2 =3D 3 ); (Wrong assignment operator) -- psql:plsqltest.sql:27: ERROR: cursor "cur1" argument 3 "param3" = provided multiple times LINE 10: ... open cur1( param3 =3D param3 , param3 :=3D 2, param2 =3D = 3 ); -- open cur1( param3 :=3D param3 , param2 =3D 3, param1 :=3D 1 ); psql:plsqltest.sql:29: ERROR: column "param2" does not exist LINE 2: ,param2 =3D 3 ^ QUERY: SELECT 1 ,param2 =3D 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. =95 Does it follow the project coding guidelines? I believe so, but someone more familiar with them will probably spot = violations better than me =3D) =95 Are there portability issues? I wouldn't know -- I don't have any experience with C portability. =95 Will it work on Windows/BSD etc? Tested under OS X, so BSD is presumably okay. No idea about other = unixes nor windows. =95 Are the comments sufficient and accurate? I'm happy enough with them. =95 Does it do what it says, correctly? Yes, excepting my comments above. =95 Does it produce compiler warnings? Yes: In file included from gram.y:12962: scan.c: In function =91yy_try_NUL_trans=92: scan.c:16243: warning: unused variable =91yyg=92 But this was not added by this patch -- it's also in the unpatched = master. =95 Can you make it crash? No. --Royce --Apple-Mail=_147B66CB-C081-476C-8B4A-C6578BE2CED1 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=windows-1252 http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php

=95 Does = the patch actually implement that?

Yes, = the feature works as advertised.

=95 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:

=95 Do we already have = it?

Not = AFAIK

=95 Does it follow SQL spec, or = the community-agreed behavior?

There's = some discussion about the syntax ( :=3D or =3D> ), I'm not sure = there's a consensus = yet.


=95 Does = it include pg_dump support (if = applicable)?

Yes -- pgplsql functions = using named parameters are correctly = dumped.

=95 Are there = dangers?

Potentially.  See = below.

=95 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 :=3D 4, 1, param1 :=3D 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 :=3D 4, = 1, param2 :=3D = 5);

and

psql:plsqltest.sql:26: ERROR:  cursor "cur1" argument 2 "param2" = provided multiple times
LINE 10:       open = cur1(param2 :=3D 4, 2, param1 :=3D = 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 : =3D 4, = 2, param1 :=3D 5); 
(notice the space between the : = and = =3D)

--

psql:plsqlt= est.sql:28: ERROR:  cursor "cur1" argument 1 "param1" provided = multiple times
LINE 11:       open cur1( param3 = =3D> 4, 2, param1 :=3D 5); 
(Wrong assignment = operator)

--

psql:p= lsqltest.sql:27: ERROR:  cursor "cur1" argument 3 "param3" provided = multiple times
LINE 10:       open cur1( 1 , = param3 :=3D 2, param2 =3D 3 );
(Wrong assignment = operator)

--

= psql:plsqltest.sql:27: ERROR:  cursor "cur1" argument 3 "param3" = provided multiple times
LINE 10: ...     open cur1( = param3 =3D param3 , param3 :=3D 2, param2 =3D 3 = );


--

      open cur1( param3 :=3D param3 , param2 =3D = 3, param1 :=3D 1 = );

psql:plsqltest.sql:29: ERROR: =  column "param2" does not exist
LINE 2: ,param2 =3D = 3
         ^
QUERY: =  SELECT 1
,param2 =3D = 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.

=95 Does = it follow the project coding = guidelines?

I believe so, but someone = more familiar with them will probably spot violations better than me = =3D)

=95 Are there portability = issues?

I wouldn't know -- I don't have = any experience with C portability.

=95 Will = it work on Windows/BSD etc?

Tested = under OS X, so BSD is presumably okay.  No idea about other unixes = nor windows.

=95 Are the comments sufficient = and accurate?

I'm happy enough with = them.

=95 Does it do what it says, = correctly?

Yes, excepting my comments = above.

=95 Does it produce compiler = warnings?

Yes:

<= div>In file included from gram.y:12962:
scan.c: In function = =91yy_try_NUL_trans=92:
scan.c:16243: warning: unused variable = =91yyg=92


But this was not = added by this patch -- it's also in the unpatched = master.

=95 Can you make it = crash?

No.

--Royce





= --Apple-Mail=_147B66CB-C081-476C-8B4A-C6578BE2CED1--