REVIEW: patch: remove redundant code from pl_exec.c

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