RE: pgbench - doCustom cleanup

Hi Fabien and Alvaro, 

I found that I have already reviewed this thread before,
so I tried to apply the patch, but part of the chunk failed,
because of the unused line below which was already removed in the
recent related commit.
>               PGResult    *res;
I removed the line and fixed the other trailing whitespaces.
See the attached latest patch.
The attached patch applies, builds cleanly,
and passes the regression tests.

On Saturday, November 24, 2018 5:58PM (GMT+9), Fabien Coelho wrote:
> About the patch you committed, a post-commit review:
>
>  - the state and function renamings are indeed a good thing.
>
>  - I'm not fond of "now = func(..., now)", I'd have just passed a
>    reference.
>
>  - I'd put out the meta commands, but keep the SQL case and the state
>    assignment in the initial function, so that all state changes are in
>    one function… which was the initial aim of the submission.
>    Kind of a compromise:-)

I have confirmed the following changes:

1.
>  - I'm not fond of "now = func(..., now)", I'd have just passed a
>    reference. 

1.1. advanceConnectionState(): 
Removed > now = doExecuteCommand(thread, st, now);
1.2. executeMetaCommand(): direct reference to state
Before:
>-				st->state = CSTATE_ABORTED;
>-				return now;
After:
>+			return CSTATE_ABORTED;

2. SQL_COMMAND type is executed in initial function advanceConnectionState(),
while META_COMMAND is executed in the subroutine executeMetaCommand().
This seems reasonable to me.

3. The function name also changed, which describes the subroutine better.
-static instr_time doExecuteCommand(TState *thread, CState *st,
-				 instr_time now);
+static ConnectionStateEnum executeMetaCommand(TState *thread, CState *st, instr_time *now);

No problems on my part as I find the changes logical.
This also needs a confirmation from Alvaro.


Regards,
Kirk Jamison