Re: [HACKERS] patch: Review handling of MOVE and FETCH (ToDo)
Pavel Stehule pavel.steh...@gmail.com writes: I am sending actualised patch as per John comment. Applied with minor fixes (mostly around MOVE ALL). 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] patch: Review handling of MOVE and FETCH (ToDo)
2009/9/29 Tom Lane t...@sss.pgh.pa.us: Pavel Stehule pavel.steh...@gmail.com writes: I am sending actualised patch as per John comment. Applied with minor fixes (mostly around MOVE ALL). thank you Pavel 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] patch: Review handling of MOVE and FETCH (ToDo)
2009/9/28 John Naylor jcnay...@gmail.com:
Pavel,
It looks good. My last email didn't go to -hackers, since I wasn't
subscribed. I had to resend to -hackers so there will be a link for
the commitfest page. I think you might have to resend your latest
patch to the list. Sorry!
nothing, patch attached
Pavel
In any case, I will say it's ready for commiter.
Thanks,
John
On Mon, Sep 28, 2009 at 2:07 AM, Pavel Stehule pavel.steh...@gmail.com
wrote:
Hello
I am sending actualised patch as per John comment.
regards
Pavel Stehule
2009/9/26 John Naylor jcnay...@gmail.com:
Hi,
Sorry, I didn't notice the attachment on Pavel's email, otherwise I
would have done this sooner! :)
I just applied and tested the new patch. Everything works great.
The only thing I would change now is some of the comments.
1). On line 289, one of the regression test comments got copied:
+ move forward in c; --should be at '5'
change to:
+ move forward in c; --should be at '1'
2). Lines 79/80:
+
errmsg(statement FETCH returns more rows.),
+
errhint(Multirows fetch are not allowed in PL/pgSQL.)));
This might sound better as statement FETCH returns multiple rows.,
and Multirow FETCH is not allowed in PL/pgSQL.
Everything else looks good to me.
John
Hi Selena and John,
Pavel's latest patch seems to address all the issues you raised in
your initial review. Do you have any comments on this new revision?
If you're happy that your issues have been resolved, please mark the
patch as Ready for Committer.
Cheers,
BJ
*** ./doc/src/sgml/plpgsql.sgml.orig 2009-09-28 09:38:55.711469112 +0200
--- ./doc/src/sgml/plpgsql.sgml 2009-09-28 09:39:24.613468923 +0200
***
*** 2656,2670
para
The options for the replaceabledirection/replaceable clause are
! the same as for commandFETCH/, namely
literalNEXT/,
literalPRIOR/,
literalFIRST/,
literalLAST/,
literalABSOLUTE/ replaceablecount/replaceable,
literalRELATIVE/ replaceablecount/replaceable,
! literalFORWARD/, or
! literalBACKWARD/.
Omitting replaceabledirection/replaceable is the same
as specifying literalNEXT/.
replaceabledirection/replaceable values that require moving
--- 2656,2670
para
The options for the replaceabledirection/replaceable clause are
! similar to commandFETCH/, namely
literalNEXT/,
literalPRIOR/,
literalFIRST/,
literalLAST/,
literalABSOLUTE/ replaceablecount/replaceable,
literalRELATIVE/ replaceablecount/replaceable,
! literalFORWARD/ optional replaceablecount/replaceable | literalALL/ /optional, or
! literalBACKWARD/ optional replaceablecount/replaceable | literalALL/ /optional.
Omitting replaceabledirection/replaceable is the same
as specifying literalNEXT/.
replaceabledirection/replaceable values that require moving
***
*** 2678,2683
--- 2678,2684
MOVE curs1;
MOVE LAST FROM curs3;
MOVE RELATIVE -2 FROM curs4;
+ MOVE FORWARD 2 FROM curs4;
/programlisting
/para
/sect3
*** ./src/pl/plpgsql/src/gram.y.orig 2009-09-28 09:38:55.713470217 +0200
--- ./src/pl/plpgsql/src/gram.y 2009-09-28 11:00:53.811467762 +0200
***
*** 72,77
--- 72,79
int until, const char *expected);
static List*read_raise_options(void);
+ static PLpgSQL_stmt_fetch *complete_direction(PLpgSQL_stmt_fetch *fetch, bool *check_FROM);
+
%}
%expect 0
***
*** 178,183
--- 180,186
* Keyword tokens
*/
%token K_ALIAS
+ %token K_ALL
%token K_ASSIGN
%token K_BEGIN
%token K_BY
***
*** 1621,1626
--- 1624,1635
if (yylex() != ';')
yyerror(syntax error);
+
+ if (fetch-returns_multiple_rows)
+ ereport(ERROR,
+ (errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg(statement FETCH returns multiple rows),
+ errhint(Multirow FETCH is not allowed in PL/pgSQL.)));
fetch-lineno = $2;
fetch-rec = rec;
***
*** 2252,2257
--- 2261,2271
}
+ /*
+ * Read FETCH or MOVE statement direction. By default,
+ * cursor will only move one row. To MOVE more than one row
+ * at a time see complete_direction().
+ */
static PLpgSQL_stmt_fetch *
read_fetch_direction(void)
{
***
*** 2269,2274
--- 2283,2289
fetch-direction = FETCH_FORWARD;
fetch-how_many = 1;
fetch-expr = NULL;
+ fetch-returns_multiple_rows = false;
/*
* Most of the direction keywords are not plpgsql keywords, so we
***
*** 2313,2323
}
else if (pg_strcasecmp(yytext, forward) == 0)
{
! /* use defaults */
}
else
Re: [HACKERS] patch: Review handling of MOVE and FETCH (ToDo)
2009/9/19 Pavel Stehule pavel.steh...@gmail.com: 2009/9/18 Selena Deckelmann selenama...@gmail.com: Hi! John Naylor and I reviewed this patch. John created two test cases to demonstrated issues described later in this email. I've attached those for reference. On Thu, Aug 27, 2009 at 8:04 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello, this small patch complete MOVE support in plpgsql and equalize plpgsql syntax with sql syntax. There are possible new directions: FORWARD expr, FORWARD ALL, BACKWARD expr, BACKWARD all. These directions are not allowed for FETCH statement, because returns more rows. This patch is related to ToDo issue: Review handling of MOVE and FETCH Hi Selena and John, Pavel's latest patch seems to address all the issues you raised in your initial review. Do you have any comments on this new revision? If you're happy that your issues have been resolved, please mark the patch as Ready for Committer. Cheers, BJ -- 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] patch: Review handling of MOVE and FETCH (ToDo)
Hello 2009/9/18 Selena Deckelmann selenama...@gmail.com: Hi! John Naylor and I reviewed this patch. John created two test cases to demonstrated issues described later in this email. I've attached those for reference. On Thu, Aug 27, 2009 at 8:04 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello, this small patch complete MOVE support in plpgsql and equalize plpgsql syntax with sql syntax. There are possible new directions: FORWARD expr, FORWARD ALL, BACKWARD expr, BACKWARD all. These directions are not allowed for FETCH statement, because returns more rows. This patch is related to ToDo issue: Review handling of MOVE and FETCH == Submission review == * Is the patch in the standard form? Yes, we have a contextual diff! * Does it apply cleanly to the current CVS HEAD? Yes! * Does it include reasonable tests, docs, etc? Suggestion: change variable 'returns_row' to 'returns_multiple_rows' and invert the values of 'returns_row' in the patch. good idea - changed Example: if (!fetch-returns_row) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg(statement FETCH returns more rows.), errhint(Multirows fetch are not allowed in PL/pgSQL.))); instead do: if (fetch-returns_multiple_rows) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg(statement FETCH returns more than one row.), errhint(Multirow FETCH is not allowed in PL/pgSQL.))); Does that make sense? In reading the code, we thought this change made this variable much easier to understand, and the affected code much easier to read. === Need a hard tab here to match the surrounding code: :) + %token K_ALL$ fixed === Can you clarify this comment? + /* + * Read FETCH or MOVE statement direction. For statement for are only + * one row directions allowed. MOVE statement can use FORWARD [(n|ALL)], + * BACKWARD [(n|ALL)] directions too. + */ We think what you mean is: By default, cursor will only move one row. To MOVE more than one row at a time see complete_direction() fixed - I add new comment there - I am sure, so this comments needs some correction from native speakers - sorry, my English is bad still. We tested on Mac OS X and Linux (Ubuntu) === Also, the tests did not test what the standard SQL syntax would require. John created a test case that demonstrated that the current patch did not work according to the SQL spec. We used that to find a little bug in complete_direction() (see below!). == Usability review == Read what the patch is supposed to do, and consider: * Does the patch actually implement that? No -- we found a bug: Line 162 of the patch: + expr = read_sql_expression2(K_FROM, K_IN, Should be: + fetch-expr = read_sql_expression2(K_FROM, K_IN, grr fixed And you don' t need to declare expr earlier in the function. Once that's changed, the regression test needs to be updated for the expected result set. * Does it follow SQL spec, or the community-agreed behavior? It conforms with the already implemented SQL syntax once the 'fetch-expr' thing is fixed. * Have all the bases been covered? We think so, as long the documentation is fixed and the above changes are applied. Another thing John noted is that additional documentation needs to be updated for the SQL standard syntax, so that it no longer says that PL/PgSQL doesn't implement the same functionality. Thanks! -selena John -- http://chesnok.com/daily - me http://endpoint.com - work Thank You Pavel *** ./doc/src/sgml/plpgsql.sgml.orig 2009-08-27 17:14:26.926410144 +0200 --- ./doc/src/sgml/plpgsql.sgml 2009-09-18 22:44:45.267608075 +0200 *** *** 2656,2670 para The options for the replaceabledirection/replaceable clause are ! the same as for commandFETCH/, namely literalNEXT/, literalPRIOR/, literalFIRST/, literalLAST/, literalABSOLUTE/ replaceablecount/replaceable, literalRELATIVE/ replaceablecount/replaceable, ! literalFORWARD/, or ! literalBACKWARD/. Omitting replaceabledirection/replaceable is the same as specifying literalNEXT/. replaceabledirection/replaceable values that require moving --- 2656,2670 para The options for the replaceabledirection/replaceable clause are ! similar to commandFETCH/, namely literalNEXT/, literalPRIOR/, literalFIRST/, literalLAST/, literalABSOLUTE/ replaceablecount/replaceable, literalRELATIVE/ replaceablecount/replaceable, ! literalFORWARD/ optional replaceablecount/replaceable | literalALL/ /optional, or ! literalBACKWARD/ optional replaceablecount/replaceable | literalALL/ /optional. Omitting replaceabledirection/replaceable is the same as specifying literalNEXT/. replaceabledirection/replaceable values that require moving *** *** 2678,2683 --- 2678,2684 MOVE curs1; MOVE LAST FROM
Re: [HACKERS] patch: Review handling of MOVE and FETCH (ToDo)
Hi! John Naylor and I reviewed this patch. John created two test cases to demonstrated issues described later in this email. I've attached those for reference. On Thu, Aug 27, 2009 at 8:04 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello, this small patch complete MOVE support in plpgsql and equalize plpgsql syntax with sql syntax. There are possible new directions: FORWARD expr, FORWARD ALL, BACKWARD expr, BACKWARD all. These directions are not allowed for FETCH statement, because returns more rows. This patch is related to ToDo issue: Review handling of MOVE and FETCH == Submission review == * Is the patch in the standard form? Yes, we have a contextual diff! * Does it apply cleanly to the current CVS HEAD? Yes! * Does it include reasonable tests, docs, etc? Suggestion: change variable 'returns_row' to 'returns_multiple_rows' and invert the values of 'returns_row' in the patch. Example: if (!fetch-returns_row) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg(statement FETCH returns more rows.), errhint(Multirows fetch are not allowed in PL/pgSQL.))); instead do: if (fetch-returns_multiple_rows) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg(statement FETCH returns more than one row.), errhint(Multirow FETCH is not allowed in PL/pgSQL.))); Does that make sense? In reading the code, we thought this change made this variable much easier to understand, and the affected code much easier to read. === Need a hard tab here to match the surrounding code: :) + %token K_ALL$ === Can you clarify this comment? + /* + * Read FETCH or MOVE statement direction. For statement for are only + * one row directions allowed. MOVE statement can use FORWARD [(n|ALL)], + * BACKWARD [(n|ALL)] directions too. + */ We think what you mean is: By default, cursor will only move one row. To MOVE more than one row at a time see complete_direction() We tested on Mac OS X and Linux (Ubuntu) === Also, the tests did not test what the standard SQL syntax would require. John created a test case that demonstrated that the current patch did not work according to the SQL spec. We used that to find a little bug in complete_direction() (see below!). == Usability review == Read what the patch is supposed to do, and consider: * Does the patch actually implement that? No -- we found a bug: Line 162 of the patch: + expr = read_sql_expression2(K_FROM, K_IN, Should be: + fetch-expr = read_sql_expression2(K_FROM, K_IN, And you don' t need to declare expr earlier in the function. Once that's changed, the regression test needs to be updated for the expected result set. * Does it follow SQL spec, or the community-agreed behavior? It conforms with the already implemented SQL syntax once the 'fetch-expr' thing is fixed. * Have all the bases been covered? We think so, as long the documentation is fixed and the above changes are applied. Another thing John noted is that additional documentation needs to be updated for the SQL standard syntax, so that it no longer says that PL/PgSQL doesn't implement the same functionality. Thanks! -selena John -- http://chesnok.com/daily - me http://endpoint.com - work MOVE_patch_test_case2 Description: Binary data MOVE_patch_test_case Description: Binary 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] patch: Review handling of MOVE and FETCH (ToDo)
ok, thank you, I'll look on these issues early regards Pavel 2009/9/18 Selena Deckelmann selenama...@gmail.com: Hi! John Naylor and I reviewed this patch. John created two test cases to demonstrated issues described later in this email. I've attached those for reference. On Thu, Aug 27, 2009 at 8:04 PM, Pavel Stehule pavel.steh...@gmail.com wrote: Hello, this small patch complete MOVE support in plpgsql and equalize plpgsql syntax with sql syntax. There are possible new directions: FORWARD expr, FORWARD ALL, BACKWARD expr, BACKWARD all. These directions are not allowed for FETCH statement, because returns more rows. This patch is related to ToDo issue: Review handling of MOVE and FETCH == Submission review == * Is the patch in the standard form? Yes, we have a contextual diff! * Does it apply cleanly to the current CVS HEAD? Yes! * Does it include reasonable tests, docs, etc? Suggestion: change variable 'returns_row' to 'returns_multiple_rows' and invert the values of 'returns_row' in the patch. Example: if (!fetch-returns_row) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg(statement FETCH returns more rows.), errhint(Multirows fetch are not allowed in PL/pgSQL.))); instead do: if (fetch-returns_multiple_rows) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg(statement FETCH returns more than one row.), errhint(Multirow FETCH is not allowed in PL/pgSQL.))); Does that make sense? In reading the code, we thought this change made this variable much easier to understand, and the affected code much easier to read. === Need a hard tab here to match the surrounding code: :) + %token K_ALL$ === Can you clarify this comment? + /* + * Read FETCH or MOVE statement direction. For statement for are only + * one row directions allowed. MOVE statement can use FORWARD [(n|ALL)], + * BACKWARD [(n|ALL)] directions too. + */ We think what you mean is: By default, cursor will only move one row. To MOVE more than one row at a time see complete_direction() We tested on Mac OS X and Linux (Ubuntu) === Also, the tests did not test what the standard SQL syntax would require. John created a test case that demonstrated that the current patch did not work according to the SQL spec. We used that to find a little bug in complete_direction() (see below!). == Usability review == Read what the patch is supposed to do, and consider: * Does the patch actually implement that? No -- we found a bug: Line 162 of the patch: + expr = read_sql_expression2(K_FROM, K_IN, Should be: + fetch-expr = read_sql_expression2(K_FROM, K_IN, And you don' t need to declare expr earlier in the function. Once that's changed, the regression test needs to be updated for the expected result set. * Does it follow SQL spec, or the community-agreed behavior? It conforms with the already implemented SQL syntax once the 'fetch-expr' thing is fixed. * Have all the bases been covered? We think so, as long the documentation is fixed and the above changes are applied. Another thing John noted is that additional documentation needs to be updated for the SQL standard syntax, so that it no longer says that PL/PgSQL doesn't implement the same functionality. Thanks! -selena John -- http://chesnok.com/daily - me http://endpoint.com - work -- 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] patch: Review handling of MOVE and FETCH (ToDo)
Pavel Stehule pavel.steh...@gmail.com wrote: this small patch complete MOVE support in plpgsql and equalize plpgsql syntax with sql syntax. Quick correction on the doc changes: s/similar as for/similar to/ -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
