RE: pgbench - doCustom cleanup - Mailing list pgsql-hackers

From Jamison, Kirk
Subject RE: pgbench - doCustom cleanup
Date
Msg-id D09B13F772D2274BB348A310EE3027C6461B47@g01jpexmbkw24
Whole thread Raw
In response to Re: pgbench - doCustom cleanup  (Fabien COELHO <coelho@cri.ensmp.fr>)
Responses RE: pgbench - doCustom cleanup
List pgsql-hackers
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

Attachment

pgsql-hackers by date:

Previous
From: Magnus Hagander
Date:
Subject: Re: Online verification of checksums
Next
From: Antonin Houska
Date:
Subject: Re: Problems with plan estimates in postgres_fdw