pgbench - doCustom cleanup

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

pgbench - doCustom cleanup

Fabien COELHO-3

Hello pgdev,

This patch rework & clarifies pgbench internal state machine.

It was indirectly triggered by Heikki review of pgbench tap tests
(https://commitfest.postgresql.org/19/1306/), and by Marina's patch about
tx retry on some errors (https://commitfest.postgresql.org/19/1645/) which
I am reviewing.

- it adds more comments to the enum state definitions and to doCustom.

- there is some code cleanup/simplifications:
   . a few useless intermediate variables are removed,
   . a macro is added to avoid a repeated pattern to set the current time,
   . performance data are always collected instead of trying to be clever
     and not collect some data in some cases.

- more fundamentally, all state changes are performed within doCustom,
   prior that there was one performed by threadRun which made undertanding
   the end of run harder. Now threadRun only look at the current state
   to make decisions about a client.

- doCustom is made to always return at the end of a script to avoid
   an infinite loop on backslash-command only script, instead of hack
   with a variable to detect loops, which made it return every two
   script runs in such cases.

- there is a small behavioral change:

   prior to the patch, a script would always run to its end once started,
   with the exception of \sleep commands which could be interrupted by
   threadRun.

   Marina's patch should enforce somehow one script = one transaction so
   that a retry makes sense, so this exception is removed to avoid
   aborting a tx implicitely.

--
Fabien.

pgbench-state-change-2.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: pgbench - doCustom cleanup

Jamison, Kirk
Hello Fabien,

I have decided to take a look into this patch.
--
patching file src/bin/pgbench/pgbench.c
Hunk #1 succeeded at 296 (offset 29 lines).
[…Snip…]
Hunk #21 succeeded at 5750 (offset 108 lines).
patching file src/include/portability/instr_time.h
….
=======================
 All 189 tests passed.
=======================
Build success

It applies cleanly, passes the tests, and builds successfully.

The simplification and refactoring of code also seems logical to me.
That said, the code looks clean and the comments are clear.
Agree that it makes sense to move all the state changes from threadRun() to doCustom() for code consistency.

I noticed in your refactored code that CSTATE_CHOOSE_SCRIPT may now also change to CSTATE_FINISHED when timer is exceeded. (Before, it only changes to either CSTATE_START_THROTTLE or CSTATE_START_TX)
But the change has not been indicated YET in the typedef enum part for CSTATE_CHOOSE_SCRIPT.
        [snip]
        /*
         * The client must first choose a script to execute.  Once chosen, it can
         * either be throttled (state CSTATE_START_THROTTLE under --rate) or start
         * right away (state CSTATE_START_TX).
         */
        CSTATE_CHOOSE_SCRIPT,

As for the behavioral change, it is assumed that there will be 1 script per transaction run.
After code reading, I interpreted the "modified" state changes below:

1. CSTATE_CHOOSE_SCRIPT may now switch CSTATE_FINISHED
Previously, it may only switch to CSTATE_THROTTLE or CSTATE_START_TX.
       
2. CSTATE_START_THROTTLE (Similar to #1) may now switch to CSTATE_FINISHED.
Previously, it may only switch to CSTATE_THROTTLE or CSTATE_START_TX.
       
3. CSTATE_START_TX: Transactions, once started, cannot be interrupted while running, and now proceeds to first command. Interrupt may only be allowed either after tx error or when tx run ends.
       
4. CSTATE_SKIP_COMMAND
No particular change in code logic, but clarified in the typedef that this state can move forward several commands until it meets next commands or finishes script.
       
5. CSTATE_END_TX: either starts over with new script (CSTATE_CHOOSE_SCRIPT) or finishes (CSTATE_FINISHED).
Previously, it either defaults to CSTATE_START_THROTTLE when choosing a new script, or finishes.


Regards,
Kirk Jamison




-----Original Message-----
From: Fabien COELHO [mailto:[hidden email]]
Sent: Saturday, August 11, 2018 6:14 PM
To: PostgreSQL Developers <[hidden email]>
Subject: pgbench - doCustom cleanup


Hello pgdev,

This patch rework & clarifies pgbench internal state machine.

It was indirectly triggered by Heikki review of pgbench tap tests (https://commitfest.postgresql.org/19/1306/), and by Marina's patch about tx retry on some errors (https://commitfest.postgresql.org/19/1645/) which I am reviewing.

- it adds more comments to the enum state definitions and to doCustom.

- there is some code cleanup/simplifications:
   . a few useless intermediate variables are removed,
   . a macro is added to avoid a repeated pattern to set the current time,
   . performance data are always collected instead of trying to be clever
     and not collect some data in some cases.

- more fundamentally, all state changes are performed within doCustom,
   prior that there was one performed by threadRun which made undertanding
   the end of run harder. Now threadRun only look at the current state
   to make decisions about a client.

- doCustom is made to always return at the end of a script to avoid
   an infinite loop on backslash-command only script, instead of hack
   with a variable to detect loops, which made it return every two
   script runs in such cases.

- there is a small behavioral change:

   prior to the patch, a script would always run to its end once started,
   with the exception of \sleep commands which could be interrupted by
   threadRun.

   Marina's patch should enforce somehow one script = one transaction so
   that a retry makes sense, so this exception is removed to avoid
   aborting a tx implicitely.

--
Fabien.


Reply | Threaded
Open this post in threaded view
|

RE: pgbench - doCustom cleanup

Fabien COELHO-3

Hello Kirk,

> I have decided to take a look into this patch.

Thanks for the feedback.

> I noticed in your refactored code that CSTATE_CHOOSE_SCRIPT may now also
> change to CSTATE_FINISHED when timer is exceeded. (Before, it only
> changes to either CSTATE_START_THROTTLE or CSTATE_START_TX) But the
> change has not been indicated YET in the typedef enum part for
> CSTATE_CHOOSE_SCRIPT.

Indeed, the comment is inaccurate and need updating.

> As for the behavioral change, it is assumed that there will be 1 script
> per transaction run. After code reading, I interpreted the "modified"
> state changes below:
>
> 1. CSTATE_CHOOSE_SCRIPT may now switch CSTATE_FINISHED
> Previously, it may only switch to CSTATE_THROTTLE or CSTATE_START_TX.

Yes, I have added a shortcut there.

> 2. CSTATE_START_THROTTLE (Similar to #1) may now switch to CSTATE_FINISHED.
> Previously, it may only switch to CSTATE_THROTTLE or CSTATE_START_TX.

This was somehow already the case before the patch in some cases, but I
moved the decision after the while throttle so that it catches more cases.


> 3. CSTATE_START_TX: Transactions, once started, cannot be interrupted
> while running, and now proceeds to first command. Interrupt may only be
> allowed either after tx error or when tx run ends.

Yes, I removed all interruptions.

> 4. CSTATE_SKIP_COMMAND
> No particular change in code logic, but clarified in the typedef that this state can move forward several commands until it meets next commands or finishes script.

Yes.

> 5. CSTATE_END_TX: either starts over with new script (CSTATE_CHOOSE_SCRIPT) or finishes (CSTATE_FINISHED).
> Previously, it either defaults to CSTATE_START_THROTTLE when choosing a new script, or finishes.

Nope, it was already jumping between CHOOSE & FINISHED, however I
simplified the logic there to avoid doCustom to stay in one client
by always returning.

Attached is a v3, where I have updated inaccurate comments.

--
Fabien.

pgbench-state-change-3.patch (23K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: pgbench - doCustom cleanup

Fabien COELHO-3

> Attached is a v3, where I have updated inaccurate comments.

Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e

--
Fabien.

pgbench-state-change-4.patch (24K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Alvaro Herrera-9
On 2018-Nov-17, Fabien COELHO wrote:

>
> > Attached is a v3, where I have updated inaccurate comments.
>
> Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e

Attached v5.  I thought that separating the part that executes the
command was an obvious readability improvement.  Tests still pass,
though maybe I broke something inadvertently.  (I started by thinking
"does this block require a 'fall-through' comment?"  The 600 line
function is pretty hard to read with the multiple messy switches and
conditionals; split into 400 + 200 subroutine it's nicer to reason
about.)

Do we really use the word "move" to talk about state changes?  It sounds
very odd to me.  I would change that to "transition" -- would anybody
object to that?  (Not changed in v5.)

On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
macro -- consider this:
        if (foo)
                INSTR_TIME_SET_CURRENT_LAZY(bar);
        else
                something_else();
Which "if" is the else now attached to?  Now maybe the C standard has an
answer for that (I don't know what it is), but it's hard to read and
likely the compiler will complain anyway.  I wrapped it in "do { }
while(0)" as is customary.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

pgbench-state-change-5.patch (26K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Fabien COELHO-3

Hello Alvaro,

Thanks for the review and improvements.

>> Attached v4 is a rebase after 409231919443984635b7ae9b7e2e261ab984eb1e
>
> Attached v5.  I thought that separating the part that executes the
> command was an obvious readability improvement.

Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
transitions to happen in doCustom's switch (st->state) and nowhere else,
which is defeated by creating the separate function.

Although it improves readability at one level, it does not help figuring
out what happens to states, which is my primary concern: The idea is that
reading doCustom is enough to build and check the automaton, which I had
to do repeatedly while reviewing Marina's patches.

Also the added doCustom comment, which announces this property becomes
false under the refactoring function:

  All state changes are performed within this function called by threadRun.

So I would suggest not to create this function.

> Do we really use the word "move" to talk about state changes?  It sounds
> very odd to me.  I would change that to "transition" -- would anybody
> object to that?  (Not changed in v5.)

Yep. I removed the goto:-)

> On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
> macro -- consider this:
> if (foo)
> INSTR_TIME_SET_CURRENT_LAZY(bar);
> else
> something_else();
> Which "if" is the else now attached to?  Now maybe the C standard has an
> answer for that (I don't know what it is), but it's hard to read and
> likely the compiler will complain anyway.  I wrapped it in "do { }
> while(0)" as is customary.
Indeed, good catch.

In the attached patched, I have I think included all your changes *but*
the separate function, for the reason discussed above, and removed the
"goto".

--
Fabien.

pgbench-state-change-6.patch (25K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Alvaro Herrera-9
On 2018-Nov-20, Fabien COELHO wrote:

> Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
> transitions to happen in doCustom's switch (st->state) and nowhere else,
> which is defeated by creating the separate function.
>
> Although it improves readability at one level, it does not help figuring out
> what happens to states, which is my primary concern: The idea is that
> reading doCustom is enough to build and check the automaton, which I had to
> do repeatedly while reviewing Marina's patches.

Yeah, there are conflicting goals here.


I didn't quite understand this hunk.  Why does it remove the
is_latencies conditional?  (The preceding comment shown here should be
updated obviously if this change is correct, but I'm not sure it is.)

@@ -3364,42 +3334,34 @@ doCustom(TState *thread, CState *st, StatsData *agg)
  /*
  * command completed: accumulate per-command execution times
  * in thread-local data structure, if per-command latencies
  * are requested.
  */
- if (is_latencies)
- {
- if (INSTR_TIME_IS_ZERO(now))
- INSTR_TIME_SET_CURRENT(now);
+ INSTR_TIME_SET_CURRENT_LAZY(now);
 
- /* XXX could use a mutex here, but we choose not to */
- command = sql_script[st->use_file].commands[st->command];
- addToSimpleStats(&command->stats,
- INSTR_TIME_GET_DOUBLE(now) -
- INSTR_TIME_GET_DOUBLE(st->stmt_begin));
- }
+ /* XXX could use a mutex here, but we choose not to */
+ command = sql_script[st->use_file].commands[st->command];
+ addToSimpleStats(&command->stats,
+ INSTR_TIME_GET_DOUBLE(now) -
+ INSTR_TIME_GET_DOUBLE(st->stmt_begin));


--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Fabien COELHO-3

> I didn't quite understand this hunk.  Why does it remove the
> is_latencies conditional?  (The preceding comment shown here should be
> updated obviously if this change is correct, but I'm not sure it is.)

Pgbench runs benches a collects performance data about it.

I simplified the code to always collect data, without trying to be clever
about cases where these data may not be useful so some collection can be
skipped.

Here the test avoids recording the statement start time, mostly a simple
assignment and then later another test avoids recording the stats in the
same case, which are mostly a few adds.

ISTM that this is over optimization and unlikely to be have any measurable
effects compared to the other tasks performed when executing commands, so
a simpler code is better.

--
Fabien.

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Alvaro Herrera-9
On 2018-Nov-20, Fabien COELHO wrote:

>
> > I didn't quite understand this hunk.  Why does it remove the
> > is_latencies conditional?  (The preceding comment shown here should be
> > updated obviously if this change is correct, but I'm not sure it is.)
>
> Pgbench runs benches a collects performance data about it.
>
> I simplified the code to always collect data, without trying to be clever
> about cases where these data may not be useful so some collection can be
> skipped.
>
> Here the test avoids recording the statement start time, mostly a simple
> assignment and then later another test avoids recording the stats in the
> same case, which are mostly a few adds.
>
> ISTM that this is over optimization and unlikely to be have any measurable
> effects compared to the other tasks performed when executing commands, so a
> simpler code is better.

I don't think we're quite ready to buy this argument just yet.
See https://www.postgresql.org/message-id/flat/31856.1400021891@...

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Alvaro Herrera-9
In reply to this post by Fabien COELHO-3
On 2018-Nov-20, Fabien COELHO wrote:

> Hmm. It is somehow, but the aim of the refactoring is to make *ALL* state
> transitions to happen in doCustom's switch (st->state) and nowhere else,
> which is defeated by creating the separate function.
>
> Although it improves readability at one level, it does not help figuring out
> what happens to states, which is my primary concern: The idea is that
> reading doCustom is enough to build and check the automaton, which I had to
> do repeatedly while reviewing Marina's patches.

I adopted your patch, then while going over it I decided to go with my
separation proposal anyway, and re-did it.

Looking at the state of the code before this patch, I totally understand
that you want to get away from the current state of affairs.  However, I
don't think my proposal makes matters worse; actually it makes them
better.  Please give the code a honest look and think whether the
separation of machine state advances is really worse with my proposal.

I also renamed some things.  doCustom() was a pretty bad name, as was
CSTATE_START_THROTTLE.

> Also the added doCustom comment, which announces this property becomes false
> under the refactoring function:
>
> All state changes are performed within this function called by threadRun.
>
> So I would suggest not to create this function.

I agree the state advances in threadRun were real messy.

Thanks for getting rid of the goto.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

pgbench-state-change-7.patch (30K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Alvaro Herrera-9
In reply to this post by Fabien COELHO-3
On 2018-Nov-20, Fabien COELHO wrote:

> > On INSTR_TIME_SET_CURRENT_LAZY(), you cannot just put an "if" inside a
> > macro -- consider this:
> > if (foo)
> > INSTR_TIME_SET_CURRENT_LAZY(bar);
> > else
> > something_else();
> > Which "if" is the else now attached to?  Now maybe the C standard has an
> > answer for that (I don't know what it is), but it's hard to read and
> > likely the compiler will complain anyway.  I wrapped it in "do { }
> > while(0)" as is customary.
>
> Indeed, good catch.

Actually, reviewing this bit again, I realized that it should be a
statement that evaluates to whether the change was made or not -- this
way, it can be used in one existing place.  Pushed that way.  (I
verified that InstrStartNode fails in the expected way if you call it
twice.)

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Alvaro Herrera-9
In reply to this post by Alvaro Herrera-9
I just pushed this.  I hope not to have upset you too much with the
subroutine thing.

Thanks for the submission and Kirk for the review.

--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: pgbench - doCustom cleanup

Fabien COELHO-3

Hello Alvaro,

> I just pushed this.  I hope not to have upset you too much with the
> subroutine thing.

Sorry for the feedback delay, my week was kind of overloaded…

Thanks for the push.

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:-)

See the attached followup patch which implements these suggestions. The
patch is quite small under "git diff -w", but larger because of the
reindentation as one "if" level is removed.

--
Fabien.

pgbench-state-change-followup-1.patch (15K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: pgbench - doCustom cleanup

Jamison, Kirk
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

pgbench-state-change-followup-2.patch (14K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

RE: pgbench - doCustom cleanup

Fabien COELHO-3

Hello Kirk,

> 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.

Indeed. Thanks for the fix.

> See the attached latest patch.
> The attached patch applies, builds cleanly,
> and passes the regression tests.

[...]

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

Ok.

You switched the patch to "waiting on author": What are you waiting from
me?

If you think that it is ok and that it should be considered by a
committer, probably Alvaro, it can be marked as "ready".

--
Fabien.

Reply | Threaded
Open this post in threaded view
|

RE: pgbench - doCustom cleanup

Jamison, Kirk
Hi Fabien,

>> See the attached latest patch.
>> The attached patch applies, builds cleanly, and passes the regression
>> tests.
>
> No problems on my part as I find the changes logical.
> This also needs a confirmation from Alvaro.
>
> Ok.
>
> You switched the patch to "waiting on author": What are you waiting from me?
>
> If you think that it is ok and that it should be considered by a committer,
> probably Alvaro, it can be marked as "ready".

Indeed, it was my mistake.
I have already marked it as "Ready for Committer".

Regards,
Kirk Jamison