REVIEW: patch: remove redundant code from pl_exec.c

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>
Cc: PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: REVIEW: patch: remove redundant code from pl_exec.c
Date: 2011-01-19 19:31:02
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


* 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

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

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:

if (isnull || !value)

rc = exec_stmts(estate, stmt->body);

You replaced the last return with:

return rc;

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

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

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.



Attachment Content-Type Size
plpgsql_change.patch text/x-diff 10.1 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Robert Haas 2011-01-19 19:39:29 Re: ToDo List Item - System Table Index Clustering
Previous Message Tom Lane 2011-01-19 19:26:02 Re: ToDo List Item - System Table Index Clustering