pgbench - doCustom cleanup

classic Classic list List threaded Threaded
3 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