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 <replaceable>direction</replaceable> clause are ! the same as for <command>FETCH</>, namely <literal>NEXT</>, <literal>PRIOR</>, <literal>FIRST</>, <literal>LAST</>, <literal>ABSOLUTE</> <replaceable>count</replaceable>, <literal>RELATIVE</> <replaceable>count</replaceable>, ! <literal>FORWARD</>, or ! <literal>BACKWARD</>. Omitting <replaceable>direction</replaceable> is the same as specifying <literal>NEXT</>. <replaceable>direction</replaceable> values that require moving --- 2656,2670 ---- <para> The options for the <replaceable>direction</replaceable> clause are ! similar to <command>FETCH</>, namely <literal>NEXT</>, <literal>PRIOR</>, <literal>FIRST</>, <literal>LAST</>, <literal>ABSOLUTE</> <replaceable>count</replaceable>, <literal>RELATIVE</> <replaceable>count</replaceable>, ! <literal>FORWARD</> <optional> <replaceable>count</replaceable> | <literal>ALL</> </optional>, or ! <literal>BACKWARD</> <optional> <replaceable>count</replaceable> | <literal>ALL</> </optional>. Omitting <replaceable>direction</replaceable> is the same as specifying <literal>NEXT</>. <replaceable>direction</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-08-26 22:43:23.138239357 +0200 --- ./src/pl/plpgsql/src/gram.y 2009-09-18 22:10:37.239609424 +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 more rows."), + errhint("Multirows fetch are 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 if (pg_strcasecmp(yytext, "backward") == 0) { fetch->direction = FETCH_BACKWARD; } else if (tok != T_SCALAR) { --- 2328,2339 ---- } else if (pg_strcasecmp(yytext, "forward") == 0) { ! fetch = complete_direction(fetch, &check_FROM); } else if (pg_strcasecmp(yytext, "backward") == 0) { fetch->direction = FETCH_BACKWARD; + fetch = complete_direction(fetch, &check_FROM); } else if (tok != T_SCALAR) { *************** *** 2346,2351 **** --- 2362,2415 ---- } + /* + * Allows directions: + * FORWARD expr, FORWARD ALL, FORWARD + * BACKWARD expr, BACKWARD ALL, BACKWARD + * + * so plpgsql should fully support PostgreSQL's MOVE statement. + */ + static PLpgSQL_stmt_fetch * + complete_direction(PLpgSQL_stmt_fetch *fetch, bool *check_FROM) + { + int tok; + + tok = yylex(); + if (tok == K_FROM || tok == K_IN) + { + *check_FROM = false; + + return fetch; + } + + if (tok == K_ALL) + { + fetch->how_many = fetch->direction == FETCH_FORWARD ? -1 : 0; + fetch->direction = FETCH_ABSOLUTE; + fetch->returns_multiple_rows = true; + *check_FROM = true; + + return fetch; + } + + plpgsql_push_back_token(tok); + fetch->expr = read_sql_expression2(K_FROM, K_IN, + "FROM or IN", + NULL); + + /* + * We don't allow multiple rows in PL/pgSQL's FETCH statement. This + * limit is enforced by syntax. Only for sure one row directions + * are allowed. Because result of expr should be greather than + * one, we expect possible multiple rows and disable this direction + * when current statement is FETCH statement. + */ + fetch->returns_multiple_rows = true; + *check_FROM = false; + + return fetch; + } + static PLpgSQL_stmt * make_return_stmt(int lineno) { *** ./src/pl/plpgsql/src/plpgsql.h.orig 2009-08-27 07:46:45.051237969 +0200 --- ./src/pl/plpgsql/src/plpgsql.h 2009-09-18 21:17:28.312609825 +0200 *************** *** 520,525 **** --- 520,526 ---- int how_many; /* count, if constant (expr is NULL) */ PLpgSQL_expr *expr; /* count, if expression */ bool is_move; /* is this a fetch or move? */ + bool returns_multiple_rows; /* returns more than one rows? */ } PLpgSQL_stmt_fetch; *** ./src/pl/plpgsql/src/scan.l.orig 2009-08-27 07:55:10.058239657 +0200 --- ./src/pl/plpgsql/src/scan.l 2009-08-27 07:55:19.387238292 +0200 *************** *** 147,152 **** --- 147,153 ---- = { return K_ASSIGN; } \.\. { return K_DOTDOT; } alias { return K_ALIAS; } + all { return K_ALL; } begin { return K_BEGIN; } by { return K_BY; } case { return K_CASE; } *** ./src/test/regress/expected/plpgsql.out.orig 2009-08-27 17:07:50.785412959 +0200 --- ./src/test/regress/expected/plpgsql.out 2009-09-18 22:36:40.000000000 +0200 *************** *** 3027,3032 **** --- 3027,3054 ---- create or replace function sc_test() returns setof integer as $$ declare + c refcursor; + x integer; + begin + open c scroll for execute 'select f1 from int4_tbl'; + fetch last from c into x; + while found loop + return next x; + move backward 2 from c; + fetch relative -1 from c into x; + end loop; + close c; + end; + $$ language plpgsql; + select * from sc_test(); + sc_test + ------------- + -2147483647 + 123456 + (2 rows) + + create or replace function sc_test() returns setof integer as $$ + declare c cursor for select * from generate_series(1, 10); x integer; begin *************** *** 3052,3057 **** --- 3074,3149 ---- 9 (3 rows) + create or replace function sc_test() returns setof integer as $$ + declare + c cursor for select * from generate_series(1, 10); + x integer; + begin + open c; + loop + move forward 2 in c; + if not found then + exit; + end if; + fetch next from c into x; + if found then + return next x; + end if; + end loop; + close c; + end; + $$ language plpgsql; + select * from sc_test(); + sc_test + --------- + 3 + 6 + 9 + (3 rows) + + drop function sc_test(); + create or replace function sc_test() returns integer + as $$ + declare + c cursor for select * from generate_series(1, 10); + x integer; + begin + open c; + move forward in c; --should be at '5' + fetch relative 0 from c into x; --fetch current position + if found then + return x; + end if; + close c; + end; + $$ language plpgsql; + select sc_test(); + sc_test + --------- + 1 + (1 row) + + create or replace function sc_test() returns integer + as $$ + declare + c cursor for select * from generate_series(1, 10); + x integer; + begin + open c; + move forward 5 in c; --should be at '5' + fetch relative 0 from c into x; --fetch current position + if found then + return x; + end if; + close c; + end; + $$ language plpgsql; + select sc_test(); + sc_test + --------- + 5 + (1 row) + drop function sc_test(); -- test qualified variable names create function pl_qual_names (param1 int) returns void as $$ *** ./src/test/regress/sql/plpgsql.sql.orig 2009-08-27 16:56:20.809413381 +0200 --- ./src/test/regress/sql/plpgsql.sql 2009-09-18 22:35:34.724609057 +0200 *************** *** 2513,2518 **** --- 2513,2536 ---- create or replace function sc_test() returns setof integer as $$ declare + c refcursor; + x integer; + begin + open c scroll for execute 'select f1 from int4_tbl'; + fetch last from c into x; + while found loop + return next x; + move backward 2 from c; + fetch relative -1 from c into x; + end loop; + close c; + end; + $$ language plpgsql; + + select * from sc_test(); + + create or replace function sc_test() returns setof integer as $$ + declare c cursor for select * from generate_series(1, 10); x integer; begin *************** *** 2533,2540 **** --- 2551,2619 ---- select * from sc_test(); + create or replace function sc_test() returns setof integer as $$ + declare + c cursor for select * from generate_series(1, 10); + x integer; + begin + open c; + loop + move forward 2 in c; + if not found then + exit; + end if; + fetch next from c into x; + if found then + return next x; + end if; + end loop; + close c; + end; + $$ language plpgsql; + + select * from sc_test(); + + drop function sc_test(); + + create or replace function sc_test() returns integer + as $$ + declare + c cursor for select * from generate_series(1, 10); + x integer; + begin + open c; + move forward in c; --should be at '5' + fetch relative 0 from c into x; --fetch current position + if found then + return x; + end if; + close c; + end; + $$ language plpgsql; + + select sc_test(); + + create or replace function sc_test() returns integer + as $$ + declare + c cursor for select * from generate_series(1, 10); + x integer; + begin + open c; + move forward 5 in c; --should be at '5' + fetch relative 0 from c into x; --fetch current position + if found then + return x; + end if; + close c; + end; + $$ language plpgsql; + + select sc_test(); + drop function sc_test(); + -- test qualified variable names create function pl_qual_names (param1 int) returns void as $$
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers