Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> The reason this is so large is that there is an entangled refactoring
> patch, splitting the exec_command() function from one giant switch()
> into one routine for each command. It's up to the committer whether to
> do it all in one patch, or to request this to be split into a
> refactoring patch plus another adding functionality on top.
Assuming we want to do it that way at all, two steps would probably be
easier to review in detail.
I'm not entirely convinced that function-per-command is an improvement
though. Seems like it would only help to the extent that you could do a
simple "return" to implement early exit, and it looks to me like that
doesn't work in a lot of places because you still have to clean up things
like malloc'd argument strings before you can return. So the question
we have to answer is whether this way looks cleaner than what we'd get if
we just changed the logic in-place. For the purpose of answering that
question, looking at the final state is the right thing to do.
I don't have a definite opinion on that core question yet, since I've not
read this version of the patch. Anybody else want to give an opinion?
regards, tom lane
