Hello
2009/9/18 Selena Deckelmann <[email protected]>:
> 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 <[email protected]>
> 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 ([email protected])
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
