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

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

Regards
Pavel Stehule

p.s. Scrollable cursors are supported yet in plpgsql. Do you know,
somebody, why this point is in ToDo (plpgsql) still?

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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

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

ok, thank you, I'll look on these issues early

regards
Pavel

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.
>
> 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
>

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

2009/9/19 Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>:
> 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

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

2009/9/28 John Naylor <jcnaylor(at)gmail(dot)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(dot)stehule(at)gmail(dot)com> wrote:
>> Hello
>>
>> I am sending actualised patch as per John comment.
>>
>> regards
>> Pavel Stehule
>>
>> 2009/9/26 John Naylor <jcnaylor(at)gmail(dot)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
>>>>
>>>
>>
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> I am sending actualised patch as per John comment.

Applied with minor fixes (mostly around MOVE ALL).

regards, tom lane

2009/9/29 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)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
>