Re: patch: Review handling of MOVE and FETCH (ToDo)

Hello

2009/9/18 Selena Deckelmann <selenamarie(at)gmail(dot)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(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.
>

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