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

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.
Interruptmay 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
untilit 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:coelho@cri.ensmp.fr] 
Sent: Saturday, August 11, 2018 6:14 PM
To: PostgreSQL Developers <pgsql-hackers@lists.postgresql.org>
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'spatch 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.