Re: 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(dot)stehule(at)gmail(dot)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