RE: pgbench - doCustom cleanup

From: "Jamison, Kirk" <k(dot)jamison(at)jp(dot)fujitsu(dot)com>
To: 'Fabien COELHO' <coelho(at)cri(dot)ensmp(dot)fr>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: PostgreSQL Developers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: RE: pgbench - doCustom cleanup
Date: 2019-03-04 07:17:06
Message-ID: D09B13F772D2274BB348A310EE3027C6461B47@g01jpexmbkw24
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: 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 Content-Type Size
pgbench-state-change-followup-2.patch application/octet-stream 10.8 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Antonin Houska 2019-03-04 07:46:17 Re: Problems with plan estimates in postgres_fdw
Previous Message Magnus Hagander 2019-03-04 07:09:33 Re: Online verification of checksums