REVIEW: patch: remove redundant code from pl_exec.c

Greetings,

* Pavel Stehule (pavel.stehule@gmail.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@snowman.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@snowman.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