Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-12-13 18:34, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@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(2, 2) 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(2, 2) 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; -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 14.12.2011 12:31, Yeb Havinga wrote: On 2011-12-13 18:34, Tom Lane wrote: Heikki Linnakangasheikki.linnakan...@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(2, 2) 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
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 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** *** 2823,2833 OPEN curs1 FOR EXECUTE 'SELECT * FROM ' || quote_ident(tabname) /para /sect3 ! sect3 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional; /synopsis para --- 2823,2833 /para /sect3 ! sect3 id=plpgsql-open-bound-cursor titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional; /synopsis para *** *** 2847,2856 OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargume --- 2847,2867 /para para + Argument values can be passed using either firsttermpositional/firstterm + or firsttermnamed/firstterm notation. In positional + notation, all arguments are specified in order. In named notation, + each argument's name is specified using literal:=/literal to + separate it from the argument expression. Similar to calling + functions, described in xref linkend=sql-syntax-calling-funcs, it + is also allowed to mix positional and named notation. + /para + + para Examples (these use the cursor declaration examples above): programlisting OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); /programlisting /para *** *** 3169,3175 COMMIT; synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis --- 3180,3186 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis *** *** 3179,3186 END LOOP optional replaceablelabel/replaceable /optional; commandFOR/ statement automatically opens the cursor, and it closes the cursor again when the loop exits. A list of actual argument value expressions must appear if and only if the cursor was declared to take ! arguments. These values will be substituted in the query, in just ! the same way as during an commandOPEN/. The variable
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Heikki Linnakangas heikki.linnakan...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-12-11 16:26, Yeb Havinga wrote: On 2011-12-06 17:58, Kevin Grittner wrote: Kevin Grittnerkgri...@wicourts.gov wrote: Yeb Havingayebhavi...@gmail.com wrote: I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. Hearing no objections -- Yeb, are you OK with doing this, and do you feel this is doable for this CF? Attach is v6 of the patch, allowing mixed mode and with new test cases in the regression tests. One difference with calling functions remains: it is allowed to place positional arguments after named parameters. Including that would add code, but nothing would be gained. Forgot to copy regression output to expected - attached v7 fixes that. -- Yeb diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml new file mode 100644 index f33cef5..ee2e3a3 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** OPEN curs1 FOR EXECUTE 'SELECT * FROM ' *** 2823,2833 /para /sect3 ! sect3 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional; /synopsis para --- 2823,2833 /para /sect3 ! sect3 id=plpgsql-open-bound-cursor titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional; /synopsis para *** OPEN replaceablebound_cursorvar/repla *** 2847,2856 --- 2847,2867 /para para + Cursors may be opened using either firsttermpositional/firstterm + or firsttermnamed/firstterm notation. Similar to calling + functions, described in xref linkend=sql-syntax-calling-funcs, it + is also allowed to mix positional and named notation. In positional + notation, all arguments are specified in order. In named notation, + each argument's name is specified using literal:=/literal to + separate it from the argument expression. + /para + + para Examples (these use the cursor declaration examples above): programlisting OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); /programlisting /para *** COMMIT; *** 3169,3186 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis The cursor variable must have been bound to some query when it was ! declared, and it emphasiscannot/ be open already. The ! commandFOR/ statement automatically opens the cursor, and it closes ! the cursor again when the loop exits. A list of actual argument value ! expressions must appear if and only if the cursor was declared to take ! arguments. These values will be substituted in the query, in just ! the same way as during an commandOPEN/. The variable replaceablerecordvar/replaceable is automatically defined as type typerecord/ and exists only inside the loop (any existing definition of the variable name is ignored within the loop). --- 3180,3203 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis The cursor variable must have been bound to some query when it was ! declared, and it emphasiscannot/ be open already. The commandFOR/ ! statement automatically opens the cursor, and it closes the cursor again ! when the loop exits. The cursor's arguments may be supplied in either ! firsttermpositional/firstterm or firsttermnamed/firstterm ! notation. A list of actual argument value expressions must appear if and ! only if the cursor was declared to take arguments. These values will be ! substituted in the query, in just the same way as during an ! commandOPEN/command described in xref ! linkend=plpgsql-open-bound-cursor. !/para ! !para The variable replaceablerecordvar/replaceable
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-12-06 17:58, Kevin Grittner wrote: Kevin Grittnerkgri...@wicourts.gov wrote: Yeb Havingayebhavi...@gmail.com wrote: I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. Hearing no objections -- Yeb, are you OK with doing this, and do you feel this is doable for this CF? Attach is v6 of the patch, allowing mixed mode and with new test cases in the regression tests. One difference with calling functions remains: it is allowed to place positional arguments after named parameters. Including that would add code, but nothing would be gained. regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-12-06 17:58, Kevin Grittner wrote: Kevin Grittnerkgri...@wicourts.gov wrote: If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. Hearing no objections -- Yeb, are you OK with doing this, and do you feel this is doable for this CF? It is not a large change, I'll be able to do it tomorrow if nothing unexpected happens, or monday otherwise. regards, Yeb -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Kevin Grittner kgri...@wicourts.gov wrote: Yeb Havinga yebhavi...@gmail.com wrote: I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. Hearing no objections -- Yeb, are you OK with doing this, and do you feel this is doable for this CF? -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Kevin, Thank you for your review! On 2011-12-03 21:53, Kevin Grittner wrote: (1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what changed. This is probably caused because I used emacs's fill-paragraph (alt+q) command, after some changes. If this is against policy, I could change the additions in such a way as to cause minor differences, however I believe that the benefit of that ends immediately after review. (b) There's one point where cursors should be possessive cursor's. (c) I think it would be less confusing to be consistent about mentioning positional and named in the same order each time. Mixing it up like that is harder to read, at least for me. Good point, both changed. (2) The error for mixing positional and named parameters should be moved up. That seems more important than too many arguments or provided multiple times if both are true. I moved the error up, though I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. (3) The -- END ADDITIONAL TESTS comment shouldn't be added to the regression tests. This is removed, also the -- START ADDITIONAL TESTS, though I kept the tests between those to comments. The way this parses out the named parameters and then builds the positional list, with some code from read_sql_construct() repeated in read_cursor_args() seems a little bit clumsy to me, but I couldn't think of a better way to do it. Same here. The new patch is attached. regards Yeb Havinga diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml new file mode 100644 index f33cef5..a3e6540 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** OPEN curs1 FOR EXECUTE 'SELECT * FROM ' *** 2823,2833 /para /sect3 ! sect3 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional; /synopsis para --- 2823,2833 /para /sect3 ! sect3 id=plpgsql-open-bound-cursor titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional; /synopsis para *** OPEN replaceablebound_cursorvar/repla *** 2847,2856 --- 2847,2867 /para para + Cursors may be opened using either firsttermpositional/firstterm + or firsttermnamed/firstterm notation. In contrast with calling + functions, described in xref linkend=sql-syntax-calling-funcs, it + is not allowed to mix positional and named notation. In positional + notation, all arguments are specified in order. In named notation, + each argument's name is specified using literal:=/literal to + separate it from the argument expression. + /para + + para Examples (these use the cursor declaration examples above): programlisting OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); /programlisting /para *** COMMIT; *** 3169,3186 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis The cursor variable must have been bound to some query when it was ! declared, and it emphasiscannot/ be open already. The ! commandFOR/ statement automatically opens the cursor, and it closes ! the cursor again when the loop exits. A list of actual argument value ! expressions must appear if and only if the cursor was declared to take ! arguments. These values will be substituted in the query, in just ! the same way as during an commandOPEN/. The variable replaceablerecordvar/replaceable is automatically defined as type typerecord/ and exists only inside the loop (any existing definition of the variable name is ignored within the loop). --- 3180,3203 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Yeb Havinga yebhavi...@gmail.com wrote: On 2011-12-03 21:53, Kevin Grittner wrote: (1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what changed. This is probably caused because I used emacs's fill-paragraph (alt+q) command, after some changes. If this is against policy, I could change the additions in such a way as to cause minor differences, however I believe that the benefit of that ends immediately after review. I don't know whether it's a hard policy, but people usually minimize whitespace changes in such situations, to make it easier for the reviewer and committer to tell what really changed. The committer can always reformat after looking that over, if they feel that's useful. The issue is small enough here that I'm not inclined to consider it a blocker for sending to the committer. (2) The error for mixing positional and named parameters should be moved up. That seems more important than too many arguments or provided multiple times if both are true. I moved the error up, though I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. If there's no technical reason not to allow them to be mixed, I would tend to favor consistency with parameter calling rules. Doing otherwise seems like to result in confusion and bug reports. How do others feel? -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Kevin Grittner kevin.gritt...@wicourts.gov wrote: Yeb Havinga yebhavi...@gmail.com wrote: I personally tend to believe it doesn't even need to be an error. There is no technical reason not to allow it. All the user needs to do is make sure that the combination of named parameters and the positional ones together are complete and not overlapping. This is also in line with calling functions, where mixing named and positional is allowed, as long as the positional arguments are first. Personally I have no opinion what is best, include the feature or give an error, and I removed the possibility during the previous commitfest. If there's no technical reason not to allow them to be mixed, I would tend to favor consistency with parameter calling rules. Doing otherwise seems like to result in confusion and bug reports. Er, that was supposed to read I would tend to favor consistency with function calling rules. As stated here: http://www.postgresql.org/docs/9.1/interactive/sql-syntax-calling-funcs.html | PostgreSQL also supports mixed notation, which combines positional | and named notation. In this case, positional parameters are | written first and named parameters appear after them. How do others feel? If there are no objections, I suggest that Yeb implement the mixed notation for cursor parameters. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
The patch is in context format, includes docs and additional regression tests, applies cleanly, passes world regression tests. The parser changes don't cause any backing up. Discussion on the list seems to have reached a consensus that this patch implements a useful feature. A previous version was reviewed. This version attempts to respond to points previously raised. Overall, it looked good, but there were a few things I would like Yeb to address before mark it is marked ready for committer. I don't think any of these will take long. (1) Doc changes: (a) These contain some unnecessary whitespace changes which make it harder to see exactly what changed. (b) There's one point where cursors should be possessive cursor's. (c) I think it would be less confusing to be consistent about mentioning positional and named in the same order each time. Mixing it up like that is harder to read, at least for me. (2) The error for mixing positional and named parameters should be moved up. That seems more important than too many arguments or provided multiple times if both are true. (3) The -- END ADDITIONAL TESTS comment shouldn't be added to the regression tests. The way this parses out the named parameters and then builds the positional list, with some code from read_sql_construct() repeated in read_cursor_args() seems a little bit clumsy to me, but I couldn't think of a better way to do it. -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-11-14 15:45, Yeb Havinga wrote: On 2011-10-15 07:41, Tom Lane wrote: Yeb Havingayebhavi...@gmail.com writes: Hello Royce, Thanks again for testing. I looked this patch over but concluded that it's not ready to apply, mainly because there are too many weird behaviors around error reporting. Thanks again for the review and comments. Attached is v3 of the patch that addresses all of the points made by Tom. In the regression test I added a section under --- START ADDITIONAL TESTS that might speedup testing. Please disregard the previous patch: besides that it contained an unused function, it turned out my statement that all of Tom's points were addressed was not true - the attached patch fixes the remaining issue of putting two kinds of errors at the correct start of the current argument location. I also put some more comments in the regression test section: mainly to assist providing testcases for review, not for permanent inclusion. To address a corner case of the form 'p1 := 1 -- comments\n, p2 := 2' it was necessary to have read_sql_construct not trim trailing whitespace, since that results in an expression of the form '1 -- comments, 2' which is wrong. regards, Yeb Havinga diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml new file mode 100644 index f33cef5..17c04d2 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** OPEN curs1 FOR EXECUTE 'SELECT * FROM ' *** 2823,2833 /para /sect3 ! sect3 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional; /synopsis para --- 2823,2833 /para /sect3 ! sect3 id=plpgsql-open-bound-cursor titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional; /synopsis para *** OPEN replaceablebound_cursorvar/repla *** 2847,2856 --- 2847,2867 /para para + Cursors may be opened using either firsttermnamed/firstterm or + firsttermpositional/firstterm notation. In contrast with calling + functions, described in xref linkend=sql-syntax-calling-funcs, it + is not allowed to mix positional and named notation. In positional + notation, all arguments are specified in order. In named notation, + each argument's name is specified using literal:=/literal to + separate it from the argument expression. + /para + + para Examples (these use the cursor declaration examples above): programlisting OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); /programlisting /para *** COMMIT; *** 3169,3186 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis The cursor variable must have been bound to some query when it was ! declared, and it emphasiscannot/ be open already. The ! commandFOR/ statement automatically opens the cursor, and it closes ! the cursor again when the loop exits. A list of actual argument value ! expressions must appear if and only if the cursor was declared to take ! arguments. These values will be substituted in the query, in just ! the same way as during an commandOPEN/. The variable replaceablerecordvar/replaceable is automatically defined as type typerecord/ and exists only inside the loop (any existing definition of the variable name is ignored within the loop). --- 3180,3203 synopsis optional lt;lt;replaceablelabel/replaceablegt;gt; /optional ! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP replaceablestatements/replaceable END LOOP optional replaceablelabel/replaceable /optional; /synopsis The cursor variable must have been bound to some query when it was ! declared, and it emphasiscannot/ be open already. The commandFOR/ ! statement automatically opens the cursor, and it closes the cursor again ! when the loop exits. The cursors arguments may be supplied in either ! firsttermnamed/firstterm or firsttermpositional/firstterm ! notation. A list of actual argument value expressions must appear if and ! only if the cursor was declared to take arguments. These values will
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-10-15 07:41, Tom Lane wrote:
Yeb Havingayebhavi...@gmail.com writes:
Hello Royce,
Thanks again for testing.
I looked this patch over but concluded that it's not ready to apply,
mainly because there are too many weird behaviors around error
reporting.
Thanks again for the review and comments. Attached is v3 of the patch
that addresses all of the points made by Tom. In the regression test I
added a section under --- START ADDITIONAL TESTS that might speedup testing.
On the documentation front, the patch includes a hunk that changes the
description of DECLARE to claim that the argument names are optional,
something I see no support for in the code. It also fails to document
that this patch affects the behavior of cursor FOR loops as well as OPEN,
since both of those places use read_cursor_args().
The declare section was removed. The cursor for loop section was changed
to include a reference to named parameters, however I was unsure about
OPEN as I was under the impression that was already altered.
regards,
Yeb Havinga
diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
new file mode 100644
index f33cef5..6a77b75
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*** OPEN curs1 FOR EXECUTE 'SELECT * FROM '
*** 2823,2833
/para
/sect3
! sect3
titleOpening a Bound Cursor/title
synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional;
/synopsis
para
--- 2823,2833
/para
/sect3
! sect3 id=plpgsql-open-bound-cursor
titleOpening a Bound Cursor/title
synopsis
! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional;
/synopsis
para
*** OPEN replaceablebound_cursorvar/repla
*** 2847,2856
--- 2847,2868
/para
para
+ Cursors that have named parameters may be opened using either
+ firsttermnamed/firstterm or firsttermpositional/firstterm
+ notation. In contrast with calling functions, described in xref
+ linkend=sql-syntax-calling-funcs, it is not allowed to mix
+ positional and named notation. In positional notation, all arguments
+ are specified in order. In named notation, each argument's name is
+ specified using literal:=/literal to separate it from the
+ argument expression.
+ /para
+
+ para
Examples (these use the cursor declaration examples above):
programlisting
OPEN curs2;
OPEN curs3(42);
+ OPEN curs3(key := 42);
/programlisting
/para
*** COMMIT;
*** 3169,3175
synopsis
optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional LOOP
replaceablestatements/replaceable
END LOOP optional replaceablelabel/replaceable /optional;
/synopsis
--- 3181,3187
synopsis
optional lt;lt;replaceablelabel/replaceablegt;gt; /optional
! FOR replaceablerecordvar/replaceable IN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional LOOP
replaceablestatements/replaceable
END LOOP optional replaceablelabel/replaceable /optional;
/synopsis
*** END LOOP optional replaceablelabel/
*** 3187,3192
--- 3199,3209
Each row returned by the cursor is successively assigned to this
record variable and the loop body is executed.
/para
+
+ para
+ See xref linkend=plpgsql-open-bound-cursor on calling cursors with
+ named notation.
+ /para
/sect2
/sect1
diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y
new file mode 100644
index 8c4c2f7..5271ab5
*** a/src/pl/plpgsql/src/gram.y
--- b/src/pl/plpgsql/src/gram.y
*** static PLpgSQL_expr *read_sql_construct(
*** 67,72
--- 67,73
const char *sqlstart,
bool isexpression,
bool valid_sql,
+ bool trim,
int *startloc,
int *endtoken);
static PLpgSQL_expr *read_sql_expression(int until,
*** for_control : for_variable K_IN
*** 1313,1318
--- 1314,1320
SELECT ,
true,
false,
+ true,
expr1loc,
tok);
*** stmt_raise : K_RAISE
*** 1692,1698
expr = read_sql_construct(',', ';', K_USING,
, or ; or USING,
SELECT ,
! true, true,
NULL, tok);
new-params
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-10-15 07:41, Tom Lane wrote: Yeb Havingayebhavi...@gmail.com writes: Hello Royce, Thanks again for testing. I looked this patch over but concluded that it's not ready to apply, mainly because there are too many weird behaviors around error reporting. Tom, thanks for reviewing - getting the syntax errors to be at the exact location was indeed something that I thought would be near impossible, however the whitespace suggestion together with the others you made seem like a good path to go forward. Thanks for taking the time to write your comments, it will be a great help with making an improved version. regards, Yeb Havinga -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Yeb Havinga yebhavi...@gmail.com writes:
Hello Royce,
Thanks again for testing.
I looked this patch over but concluded that it's not ready to apply,
mainly because there are too many weird behaviors around error
reporting.
The biggest problem is that the patch cuts up and reassembles the source
text and then hands that off to check_sql_expr, ignoring the latter's
comment that says
* It is assumed that stmt represents a copy of the function source text
* beginning at offset location, with leader text of length leaderlen
* (typically SELECT ) prefixed to the source text. We use this assumption
* to transpose any error cursor position back to the function source text.
This means that syntax error positioning for errors in the argument
expressions is generally pretty wacko, but especially so if the
arguments are supplied in other than left-to-right order. An example is
create or replace function fooey() returns void as $$
declare
c1 cursor (p1 int, p2 int) for
select * from tenk1 where thousand = p1 and tenthous = p2;
begin
open c1 ( p2 := 42/, p1 := 77);
end $$ language plpgsql;
which gives this:
ERROR: syntax error at or near ;
LINE 6: open c1 ( p2 := 42/, p1 := 77);
^
which is not going to impress anybody as helpful, either as to the message
text (the user didn't write any ; nearby) or as to the cursor
positioning. And it doesn't look very much more professional if the error
is run-time rather than parse-time:
create or replace function fooey() returns void as $$
declare
c1 cursor (p1 int, p2 int) for
select * from tenk1 where thousand = p1 and tenthous = p2;
begin
open c1 ( p2 := 42/0, p1 := 77);
end $$ language plpgsql;
select fooey();
ERROR: division by zero
CONTEXT: SQL statement SELECT 77
,42/0;
PL/pgSQL function fooey line 6 at OPEN
where again you're displaying something almost completely unlike what the
user wrote.
I don't have any great suggestions about how to fix this :-(. Perhaps
it'd be acceptable to copy the argument-list text into the query string,
carefully replacing the parameters and := marks with appropriate amounts
of whitespace (the same number of characters, not the same number of
bytes). This would allow syntax errors from check_sql_expr to match up
correctly, and runtime errors would at least show a string that doesn't
look totally unlike what the user wrote. But it would be painful to get
the values assigned to the cursor parameters in the right order --- I
think most likely you'd need to generate a new row list having the
cursor parameter values in the order written in the OPEN.
Also, I concur with Royce that it would be a whole lot better to apply
check_sql_expr to each argument expression as soon as you've parsed it
off, so that typos like p1 : = 42 get detected soon enough to be
helpful, rather than ending up with misleading error messages. Very
possibly that would also simplify getting parse-time syntax errors to
point to the right places, since you'd be calling check_sql_expr on text
not far separated from the source query. (In fact, personally I'd use
read_sql_expression2(',', ')', ...) to parse off each expression and check
it immediately.) Maybe with that fix, it'd be all right if the run-time
query rearranged the expressions into the order required by the cursor
... though I'd still counsel spending more sweat on making the run-time
string look nicer. Something like
ERROR: division by zero
CONTEXT: SQL statement SELECT /* p1 := */ 77 , /* p2 := */ 42/0
PL/pgSQL function fooey line 6 at OPEN
would be easier for users to make sense of, I think.
By accident I noticed that the parameter name matching is inaccurate,
for instance this fails to fail:
create or replace function fooey() returns void as $$
declare
c1 cursor (p1 int, p2 int) for
select * from tenk1 where thousand = p1 and tenthous = p2;
begin
open c1 ( p1 := 42, p2z := 77);
end $$ language plpgsql;
select fooey();
I think this is because of the use of strncmp() --- is that really needed
rather than just plain strcmp()?
Cursor positioning for some errors is a bit flaky, observe these cases:
ERROR: mixing positional and named parameter assignment not allowed in cursor
c1
LINE 6: open c1 ( p2 : = 42, p2 := 77);
^
ERROR: cursor c1 argument 2 p2 provided multiple times
LINE 6: open c1 ( p2 := 42, p2 := 77);
^
In both of these cases I'd have expected the syntax cursor to point at
the second p2. I'd lose the 2 in that second message text, as well
--- the cursor argument name is sufficient, and as-is it does not read
very nicely.
On the documentation front, the patch includes a hunk that changes the
description of DECLARE to claim that the argument names are optional,
something I see no support for in the code. It also fails to document
that this patch affects the behavior of cursor FOR loops as well as OPEN,
since both of those places use read_cursor_args().
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 08/10/2011, at 1:56 AM, Yeb Havinga wrote: Attach is v2 of the patch. Mixed notation now raises an error. In contrast with what I said above, named parameter related errors are thrown before any syntax errors. I tested with raising syntax errors first but the resulting code was a bit more ugly and the sql checking under a error condition (i.e. double named parameter error means there is one parameter in short) was causing serious errors. Documentation was also added, regression tests updated. I've tested this patch out and can confirm mixed notation produces an error: psql:plsqltest.sql:27: ERROR: mixing positional and named parameter assignment not allowed in cursor cur1 LINE 10: open cur1( param2 := 4, 2, 5); Just one small thing: it'd be nice to have an example for cursor declaration with named parameters. Your patch adds one for opening a cursor, but not for declaring one. Other than that, I think the patch is good. Everything works as advertised =) --Royce -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Hello Royce, Thanks again for testing. On 2011-10-11 13:55, Royce Ausburn wrote: Just one small thing: it'd be nice to have an example for cursor declaration with named parameters. Your patch adds one for opening a cursor, but not for declaring one. Declaration of cursors with named parameters is already part of PostgreSQL (so it is possible to use the parameter names in the cursor query instead of $1, $2, etc.) and it also already documented with an example, just a few lines above the open examples. See curs3 on http://developer.postgresql.org/pgdocs/postgres/plpgsql-cursors.html Other than that, I think the patch is good. Everything works as advertised =) Thanks! -- Yeb Havinga http://www.mgrid.net/ Mastering Medical Data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 11/10/2011, at 11:38 PM, Yeb Havinga wrote: Declaration of cursors with named parameters is already part of PostgreSQL (so it is possible to use the parameter names in the cursor query instead of $1, $2, etc.) and it also already documented with an example, just a few lines above the open examples. See curs3 on http://developer.postgresql.org/pgdocs/postgres/plpgsql-cursors.html Doh - my apologies!
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On 2011-10-07 12:21, Yeb Havinga wrote: On 2011-10-06 16:04, Royce Ausburn wrote: Initial Review for patch: http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php 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. Attach is v2 of the patch. Mixed notation now raises an error. In contrast with what I said above, named parameter related errors are thrown before any syntax errors. I tested with raising syntax errors first but the resulting code was a bit more ugly and the sql checking under a error condition (i.e. double named parameter error means there is one parameter in short) was causing serious errors. Documentation was also added, regression tests updated. regards, -- Yeb Havinga http://www.mgrid.net/ Mastering Medical Data diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml new file mode 100644 index c14c34c..45081f8 *** a/doc/src/sgml/plpgsql.sgml --- b/doc/src/sgml/plpgsql.sgml *** END; *** 2699,2718 Another way is to use the cursor declaration syntax, which in general is: synopsis ! replaceablename/replaceable optional optional NO /optional SCROLL /optional CURSOR optional ( replaceablearguments/replaceable ) /optional FOR replaceablequery/replaceable; /synopsis (literalFOR/ can be replaced by literalIS/ for ! productnameOracle/productname compatibility.) ! If literalSCROLL/ is specified, the cursor will be capable of ! scrolling backward; if literalNO SCROLL/ is specified, backward ! fetches will be rejected; if neither specification appears, it is ! query-dependent whether backward fetches will be allowed. ! replaceablearguments/replaceable, if specified, is a ! comma-separated list of pairs literalreplaceablename/replaceable ! replaceabledatatype/replaceable/literal that define names to be ! replaced by parameter values in the given query. The actual ! values to substitute for these names will be specified later, ! when the cursor is opened. /para para Some examples: --- 2699,2717 Another way is to use the cursor declaration syntax, which in general is: synopsis ! replaceablename/replaceable optional optional NO /optional SCROLL /optional CURSOR optional ( optional replaceableargname/replaceable /optional replaceableargtype/replaceable optional, .../optional) /optional FOR replaceablequery/replaceable; /synopsis (literalFOR/ can be replaced by literalIS/ for ! productnameOracle/productname compatibility.) If literalSCROLL/ ! is specified, the cursor will be capable of scrolling backward; if ! literalNO SCROLL/ is specified, backward fetches will be rejected; if ! neither specification appears, it is query-dependent whether backward ! fetches will be allowed. replaceableargname/replaceable, if ! specified, defines the name to be replaced by parameter values in the ! given query. The actual values to substitute for these names will be ! specified later, when the cursor is opened. ! literalreplaceableargtype/replaceable/literal defines the datatype ! of the parameter. /para para Some examples: *** OPEN curs1 FOR EXECUTE 'SELECT * FROM ' *** 2827,2833 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( replaceableargument_values/replaceable ) /optional; /synopsis para --- 2826,2832 titleOpening a Bound Cursor/title synopsis ! OPEN replaceablebound_cursorvar/replaceable optional ( optional replaceableargname/replaceable := /optional replaceableargument_value/replaceable optional, .../optional ) /optional; /synopsis para *** OPEN replaceablebound_cursorvar/repla *** 2854,2864 --- 2853,2875 commandOPEN/. /para + para + Cursors that have named parameters may be opened using either + firsttermnamed/firstterm or firsttermpositional/firstterm + notation. In contrast with calling functions, described in xref + linkend=sql-syntax-calling-funcs, it is not allowed to mix + positional and named notation. In positional notation, all arguments + are specified in order. In named notation, each argument's name is + specified using literal:=/literal to separate it from the + argument expression. + /para + para Examples: programlisting OPEN curs2; OPEN curs3(42); + OPEN curs3(key := 42); /programlisting /para /sect3 diff --git a/src/pl/plpgsql/src/gram.y b/src/pl/plpgsql/src/gram.y new file mode 100644 index f8e956b..b9bf888 ***
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Royce Ausburn royce...@inomial.com writes: Initial Review for patch: http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php The patch adds a means of specifying named cursor parameter arguments in pg/plsql. 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 I still think what I said in that message, which is that it's premature to add this syntax to plpgsql cursors when we have thoughts of changing it. There is not any groundswell of demand from the field for named parameters to cursors, so I think we can just leave this in abeyance until the function case has settled. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On Thu, Oct 6, 2011 at 12:23 PM, Tom Lane t...@sss.pgh.pa.us wrote: Royce Ausburn royce...@inomial.com writes: Initial Review for patch: http://archives.postgresql.org/pgsql-hackers/2011-09/msg00744.php The patch adds a means of specifying named cursor parameter arguments in pg/plsql. • 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 I still think what I said in that message, which is that it's premature to add this syntax to plpgsql cursors when we have thoughts of changing it. There is not any groundswell of demand from the field for named parameters to cursors, so I think we can just leave this in abeyance until the function case has settled. +1. However, if that's the route we're traveling down, I think we had better go ahead and remove the one remaining = operator from hstore in 9.2: CREATE OPERATOR = ( LEFTARG = text, RIGHTARG = text, PROCEDURE = hstore ); We've been warning that this operator name was deprecated since 9.0, so it's probably about time to take the next step, if we want to have a chance of getting this sorted out in finite time. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Robert Haas robertmh...@gmail.com writes: +1. However, if that's the route we're traveling down, I think we had better go ahead and remove the one remaining = operator from hstore in 9.2: Fair enough. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On Oct 6, 2011, at 9:46 AM, Tom Lane wrote: +1. However, if that's the route we're traveling down, I think we had better go ahead and remove the one remaining = operator from hstore in 9.2: Fair enough. Would it then be added as an alias for := for named function parameters? Or would that come still later? Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
David E. Wheeler da...@kineticode.com writes: On Oct 6, 2011, at 9:46 AM, Tom Lane wrote: +1. However, if that's the route we're traveling down, I think we had better go ahead and remove the one remaining = operator from hstore in 9.2: Fair enough. Would it then be added as an alias for := for named function parameters? Or would that come still later? Once we do that, it will be impossible not merely deprecated to use = as an operator name. I think that has to wait at least another release cycle or two past where we're using it ourselves. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On Oct 6, 2011, at 10:37 AM, Tom Lane wrote: Would it then be added as an alias for := for named function parameters? Or would that come still later? Once we do that, it will be impossible not merely deprecated to use = as an operator name. I think that has to wait at least another release cycle or two past where we're using it ourselves. Okay. I kind of like := so there's no rush AFAIC. :-) David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
David E. Wheeler da...@kineticode.com writes: Would it then be added as an alias for := for named function parameters? Or would that come still later? Once we do that, it will be impossible not merely deprecated to use = as an operator name. I think that has to wait at least another release cycle or two past where we're using it ourselves. Okay. I kind of like := so there's no rush AFAIC. :-) Hmm ... actually, that raises another issue that I'm not sure whether there's consensus for or not. Are we intending to keep name := value syntax forever, as an alternative to the standard name = value syntax? I can't immediately see a reason not to, other than the it's not standard argument. Because if we *are* going to keep it forever, there's no very good reason why we shouldn't accept this plpgsql cursor patch now. We'd just have to remember to extend plpgsql to take = at the same time we do that for core function calls. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On Oct 6, 2011, at 10:46 AM, Tom Lane wrote: Okay. I kind of like := so there's no rush AFAIC. :-) Hmm ... actually, that raises another issue that I'm not sure whether there's consensus for or not. Are we intending to keep name := value syntax forever, as an alternative to the standard name = value syntax? I can't immediately see a reason not to, other than the it's not standard argument. The only reason it would be required, I think, is if the SQL standard developed some other use for that operator. Because if we *are* going to keep it forever, there's no very good reason why we shouldn't accept this plpgsql cursor patch now. We'd just have to remember to extend plpgsql to take = at the same time we do that for core function calls. Makes sense. Best, David -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
On Thu, Oct 6, 2011 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote: David E. Wheeler da...@kineticode.com writes: Would it then be added as an alias for := for named function parameters? Or would that come still later? Once we do that, it will be impossible not merely deprecated to use = as an operator name. I think that has to wait at least another release cycle or two past where we're using it ourselves. Okay. I kind of like := so there's no rush AFAIC. :-) Hmm ... actually, that raises another issue that I'm not sure whether there's consensus for or not. Are we intending to keep name := value syntax forever, as an alternative to the standard name = value syntax? I can't immediately see a reason not to, other than the it's not standard argument. Because if we *are* going to keep it forever, there's no very good reason why we shouldn't accept this plpgsql cursor patch now. We'd just have to remember to extend plpgsql to take = at the same time we do that for core function calls. It's hard to see adding support for = and dropping support for := in the same release. That would be a compatibility nightmare. If := is used by the standard for some other, incompatible purpose, then I suppose we would want to add support for =, wait a few releases, deprecate :=, wait a couple of releases, remove := altogether. But IIRC we picked := precisely because the standard didn't use it at all, or at least not for anything related... in which case we may as well keep it around more or less indefinitely. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
2011/10/6 Robert Haas robertmh...@gmail.com: On Thu, Oct 6, 2011 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote: David E. Wheeler da...@kineticode.com writes: Would it then be added as an alias for := for named function parameters? Or would that come still later? Once we do that, it will be impossible not merely deprecated to use = as an operator name. I think that has to wait at least another release cycle or two past where we're using it ourselves. Okay. I kind of like := so there's no rush AFAIC. :-) Hmm ... actually, that raises another issue that I'm not sure whether there's consensus for or not. Are we intending to keep name := value syntax forever, as an alternative to the standard name = value syntax? I can't immediately see a reason not to, other than the it's not standard argument. Because if we *are* going to keep it forever, there's no very good reason why we shouldn't accept this plpgsql cursor patch now. We'd just have to remember to extend plpgsql to take = at the same time we do that for core function calls. It's hard to see adding support for = and dropping support for := in the same release. That would be a compatibility nightmare. If := is used by the standard for some other, incompatible purpose, then I suppose we would want to add support for =, wait a few releases, deprecate :=, wait a couple of releases, remove := altogether. But IIRC we picked := precisely because the standard didn't use it at all, or at least not for anything related... in which case we may as well keep it around more or less indefinitely. +1 Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [REVIEW] Patch for cursor calling with named parameters
Forgive my ignorance -- do I need to be doing anything else now seeing as I started the review? On 07/10/2011, at 7:15 AM, Pavel Stehule wrote: 2011/10/6 Robert Haas robertmh...@gmail.com: On Thu, Oct 6, 2011 at 1:46 PM, Tom Lane t...@sss.pgh.pa.us wrote: David E. Wheeler da...@kineticode.com writes: Would it then be added as an alias for := for named function parameters? Or would that come still later? Once we do that, it will be impossible not merely deprecated to use = as an operator name. I think that has to wait at least another release cycle or two past where we're using it ourselves. Okay. I kind of like := so there's no rush AFAIC. :-) Hmm ... actually, that raises another issue that I'm not sure whether there's consensus for or not. Are we intending to keep name := value syntax forever, as an alternative to the standard name = value syntax? I can't immediately see a reason not to, other than the it's not standard argument. Because if we *are* going to keep it forever, there's no very good reason why we shouldn't accept this plpgsql cursor patch now. We'd just have to remember to extend plpgsql to take = at the same time we do that for core function calls. It's hard to see adding support for = and dropping support for := in the same release. That would be a compatibility nightmare. If := is used by the standard for some other, incompatible purpose, then I suppose we would want to add support for =, wait a few releases, deprecate :=, wait a couple of releases, remove := altogether. But IIRC we picked := precisely because the standard didn't use it at all, or at least not for anything related... in which case we may as well keep it around more or less indefinitely. +1 Pavel -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
