REVIEW: patch: remove redundant code from pl_exec.c

Hello

This patch remove redundant rows from PL/pgSQL executor (-89 lines).
Doesn't change a functionality.

Regards

Pavel Stehule

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> This patch remove redundant rows from PL/pgSQL executor (-89 lines).
> Doesn't change a functionality.

I'm not really impressed with this idea: there's no a priori reason
that all those loop types would necessarily have exactly the same
control logic.

regards, tom lane

Excerpts from Pavel Stehule's message of vie dic 17 07:02:00 -0300 2010:
> Hello
>
> This patch remove redundant rows from PL/pgSQL executor (-89 lines).
> Doesn't change a functionality.

Hmm I'm not sure but I think the new code has some of the result values
inverted. Did you test this thoroughly? I think this would be a nice
simplification because the repetitive coding is ugly and confusing, but
I'm nervous about the unstated assumption that all loop structs are
castable to the new struct. Seems like it could be easily broken in the
future.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

2010/12/17 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> This patch remove redundant rows from PL/pgSQL executor (-89 lines).
>> Doesn't change a functionality.
>
> I'm not really impressed with this idea: there's no a priori reason
> that all those loop types would necessarily have exactly the same
> control logic.
>

this code processes a rc from EXIT, CONTINUE and RETURN statement. All
these statements are defined independent to outer loops, so there are
no reason why this code has be different. And actually removed code
was almost same. There was different a process for FOR statement,
because there isn't possible direct "return" from cycle, because is
necessary to release a allocated memory.

There is no reason why the processing should be same, but actually is same.

>                        regards, tom lane
>

2010/12/17 Alvaro Herrera <alvherre(at)commandprompt(dot)com>:
> Excerpts from Pavel Stehule's message of vie dic 17 07:02:00 -0300 2010:
>> Hello
>>
>> This patch remove redundant rows from PL/pgSQL executor (-89 lines).
>> Doesn't change a functionality.
>
> Hmm I'm not sure but I think the new code has some of the result values
> inverted.  Did you test this thoroughly?  I think this would be a nice
> simplification because the repetitive coding is ugly and confusing, but
> I'm nervous about the unstated assumption that all loop structs are
> castable to the new struct.  Seems like it could be easily broken in the
> future.
>

All regress tests was successful.

A common structure isn't a new. There is same for FOR loops, there is
some similar in parser yylval, and it is only explicit description of
used construction for stmt structures. I should not to modify any
other structure. But I am not strong in this. A interface can be
changed and enhanced about pointer to label. Just I am not satisfied
from current state, where same things are implemented with minimal
difference.

Pavel

> --
> Álvaro Herrera <alvherre(at)commandprompt(dot)com>
> The PostgreSQL Company - Command Prompt, Inc.
> PostgreSQL Replication, Consulting, Custom Development, 24x7 support
>

Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
> 2010/12/17 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>> I'm not really impressed with this idea: there's no a priori reason
>> that all those loop types would necessarily have exactly the same
>> control logic.

> There is no reason why the processing should be same, but actually is same.

Yes, and it might need to be different in future, whereupon this patch
would make life extremely difficult.

regards, tom lane

2010/12/17 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
> Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com> writes:
>> 2010/12/17 Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>:
>>> I'm not really impressed with this idea: there's no a priori reason
>>> that all those loop types would necessarily have exactly the same
>>> control logic.
>
>> There is no reason why the processing should be same, but actually is same.
>
> Yes, and it might need to be different in future, whereupon this patch
> would make life extremely difficult.

Do you know about some possible change?

I can't to argument with this argument. But I can use a same argument.
Isn't be more practical to have a centralized management for return
code? There is same probability to be some features in future that
will need a modify this fragment - for example a new return code
value. Without centralized management, you will have to modify four
fragments instead one.

Regards

Pavel Stehule

>
>                        regards, tom lane
>

Greetings,

* Pavel Stehule (pavel(dot)stehule(at)gmail(dot)com) wrote:
> This patch remove redundant rows from PL/pgSQL executor (-89 lines).

While I can certainly appreciate wanting to remove redundant code, I
don't think this change actually improves the situation. The problem is
more than just that we might want to make a change to 'while' but not
'for', it's also that it makes it very difficult to follow the code
flow.

If you could have found a way to make it work for all calls to
exec_stmts(), it might be a bit better, but you can't without kludging
it up further. If you could, then it might be possible to move some of
this logic *into* exec_stmts(), but I don't see that being reasonable
either.

In the end, I'd recommend cleaning up the handling of the exec_stmts()
return code so that all of these pieces follow the same style and look
similar (I'd go with the switch-based approach and remove the if/else
branches). That'll make it easier for anyone coming along later who
does end up needing to change all three.

> Doesn't change a functionality.

I'm not convinced of this either, to be honest.. In exec_stmt_while(),
there is:

for(;;)
{
[...]
if (isnull || !value)
break;

rc = exec_stmts(estate, stmt->body);
[...]
}
return PLPGSQL_RC_OK;

You replaced the last return with:

return rc;

Which could mean returning an uninitialized rc if the above break;
happens.

In the end, I guess it's up to the committers on how they feel about
this, so here's an updated patch which fixes the above issue and
improves the comments/grammer. Passes all regressions and works in my
limited testing.

commit e6639d83db5b301e184bf158b1591007a3f79526
Author: Stephen Frost <sfrost(at)snowman(dot)net>
Date: Wed Jan 19 14:28:36 2011 -0500

Improve comments in pl_exec.c wrt can_leave_loop()

This patch improves some of the comments around can_leave_loop(), and
fixes a couple of fall-through cases to make sure they behave the same
as before the code de-duplication patch that introduced
can_leave_loop().

commit f8262adcba662164dbc24d289cb036b3f0aee582
Author: Stephen Frost <sfrost(at)snowman(dot)net>
Date: Wed Jan 19 14:26:27 2011 -0500

remove redundant code from pl_exec.c

This patch removes redundant handling of exec_stmts()'s return code
by creating a new function to be called after exec_stmts() is run.
This new function will then handle the return code, update it if
necessary, and return if the loop should continue or not.

Patch by Pavel Stehule.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> While I can certainly appreciate wanting to remove redundant code, I
> don't think this change actually improves the situation. The problem is
> more than just that we might want to make a change to 'while' but not
> 'for', it's also that it makes it very difficult to follow the code
> flow.

That was my reaction as well; and I was also concerned that this could
have a non-negligible performance price. (At the very least it's adding
an additional function call per loop execution, and there could also be
a penalty from forcing "rc" to be in memory rather than a register.)

I think we should reject this one.

> In the end, I'd recommend cleaning up the handling of the exec_stmts()
> return code so that all of these pieces follow the same style and look
> similar (I'd go with the switch-based approach and remove the if/else
> branches). That'll make it easier for anyone coming along later who
> does end up needing to change all three.

Using a switch there is a bit problematic since in some cases you want
to use "break" to exit the loop. We could replace such breaks by gotos,
but that would be another strike against the argument that you're making
things more readable. I think the switch in exec_stmt_loop is only
workable because it has no cleanup to do, so it can just "return" in
places where a loop break would otherwise be needed. In short: if you
want to make these all look alike, better to go the other way.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> I think we should reject this one.

Works for me.

> Using a switch there is a bit problematic since in some cases you want
> to use "break" to exit the loop. We could replace such breaks by gotos,
> but that would be another strike against the argument that you're making
> things more readable. I think the switch in exec_stmt_loop is only
> workable because it has no cleanup to do, so it can just "return" in
> places where a loop break would otherwise be needed. In short: if you
> want to make these all look alike, better to go the other way.

Ah, yeah, good point. We do use gotos elsewhere for this reason, might
consider revisiting those also, if we're trying to a 'clean-up' patch.
In any case, I'll mark this as rejected.

Thanks!

Stephen

2011/1/19 Stephen Frost <sfrost(at)snowman(dot)net>:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>> I think we should reject this one.
>
> Works for me.
>
>> Using a switch there is a bit problematic since in some cases you want
>> to use "break" to exit the loop.  We could replace such breaks by gotos,
>> but that would be another strike against the argument that you're making
>> things more readable.  I think the switch in exec_stmt_loop is only
>> workable because it has no cleanup to do, so it can just "return" in
>> places where a loop break would otherwise be needed.  In short: if you
>> want to make these all look alike, better to go the other way.
>
> Ah, yeah, good point.  We do use gotos elsewhere for this reason, might
> consider revisiting those also, if we're trying to a 'clean-up' patch.
> In any case, I'll mark this as rejected.

ok, I don't thinking so current code is readable, but I can't to do better now.

Thank you for review.

Regards

Pavel Stehule

>
>        Thanks!
>
>                Stephen
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.10 (GNU/Linux)
>
> iEYEARECAAYFAk03S10ACgkQrzgMPqB3kigWdQCeIU/dvgJ8bMVZ7nh+TYiFHVlP
> 4H4AnR/JbboMWbCu95G2aUEcP3LZDDGM
> =R8c6
> -----END PGP SIGNATURE-----
>
>