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
