Re: REVIEW: patch: remove redundant code from pl_exec.c

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: Pavel Stehule <pavel(dot)stehule(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: REVIEW: patch: remove redundant code from pl_exec.c
Date: 2011-01-19 20:33:57
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Stephen Frost 2011-01-19 20:36:45 Re: REVIEW: patch: remove redundant code from pl_exec.c
Previous Message Robert Haas 2011-01-19 20:19:24 Re: ToDo List Item - System Table Index Clustering