[PATCH] Refactor pgbench to make future improvements easier

Started by Hannu Krosing3 days ago4 messageshackers

hannu@tm.ee

3 days ago

Hi hackers,

`src/bin/pgbench/pgbench.c` has grown into a ~8,150-line monolithic source
file combining benchmark statistics, variable management, socket event
multiplexing, script/expression parsing and evaluation, meta-command
handlers, schema/data initialization, and the multi-threaded client state
machine.

This 7-patch series splits `src/bin/pgbench/pgbench.c` into focused,
self-contained compilation units while preserving exact behavior and all
existing tests:

1. `stats.c/h`: Extract `SimpleStats` and `StatsData` structures, latency
and retry accumulation, and formatting helpers.
2. `variable.c/h`: Extract `PgBenchValue`, `Variable`, and `Variables`
management, binary-search variable lookup, type coercion, and SQL query
variable substitution (`assignVariables` / `getQueryParams`).
3. `poller.c/h`: Extract the `socket_set` abstraction and platform-specific
`ppoll(2)` / `select(2)` socket multiplexing implementation.
4. `script.c/h`: Extract built-in script definitions, SQL and backslash
meta-command parsing, Bison/Flex expression parser callbacks, and
runtime expression evaluation (`evaluateExpr`, random distributions,
hashing, and `permute`).
5. `commands.c/h`: Extract runtime meta-command execution (`executeMetaCommand`,
`\set`, `\sleep`, `\if`/`\elif`/`\else`/`\endif`, `\shell`/`\setshell`),
`\gset`/`\aset` result processing, and pipeline command preparation.
6. `init.c/h`: Extract `-i` initialization routines (`runInitSteps`), DDL
table/index/partition creation, client-side and server-side data
generation, and vacuum helpers.
7. `engine.c/h`: Extract the client connection state machine
(`advanceConnectionState`), error/retry handling, worker thread loop
(`threadRun`), progress/log reporting, and final results reporting,
leaving `pgbench.c` (~978 lines) dedicated to CLI option parsing (`main`
and `usage`).

Each step builds cleanly with both Make and Meson and passes all 686 TAP
tests in `src/bin/pgbench`.

This modularization also prepares a clean boundary for extracting pgbench's
random number distributions, hashing, and permutation routines into
`src/common` (`pgbench_funcs.c/h`) and exposing them in SQL via
`contrib/pgbench`.

Hannu Krosing (7):
Refactor pgbench: extract statistics and latency tracking into
stats.c/h
Refactor pgbench: extract scoped variable store and value evaluation
into variable.c/h
Refactor pgbench: extract socket multiplexing and event poller into
poller.c/h
Refactor pgbench: extract script parsing and expression evaluation
into script.c/h
Refactor pgbench: extract meta-command execution and handlers into
commands.c/h
Refactor pgbench: extract initialization, schema, and data generators
into init.c/h
Refactor pgbench: extract execution engine, state machine, and thread
runner into engine.c/h

src/bin/pgbench/Makefile | 9 +-
src/bin/pgbench/commands.c | 737 ++++
src/bin/pgbench/commands.h | 70 +
src/bin/pgbench/engine.c | 2141 +++++++++++
src/bin/pgbench/engine.h | 161 +
src/bin/pgbench/init.c | 786 ++++
src/bin/pgbench/init.h | 66 +
src/bin/pgbench/meson.build | 7 +
src/bin/pgbench/pgbench.c | 7170 +----------------------------------
src/bin/pgbench/pgbench.h | 218 +-
src/bin/pgbench/poller.c | 196 +
src/bin/pgbench/poller.h | 48 +
src/bin/pgbench/script.c | 1973 ++++++++++
src/bin/pgbench/script.h | 253 ++
src/bin/pgbench/stats.c | 203 +
src/bin/pgbench/stats.h | 172 +
src/bin/pgbench/variable.c | 834 ++++
src/bin/pgbench/variable.h | 130 +
18 files changed, 7902 insertions(+), 7272 deletions(-)
create mode 100644 src/bin/pgbench/commands.c
create mode 100644 src/bin/pgbench/commands.h
create mode 100644 src/bin/pgbench/engine.c
create mode 100644 src/bin/pgbench/engine.h
create mode 100644 src/bin/pgbench/init.c
create mode 100644 src/bin/pgbench/init.h
create mode 100644 src/bin/pgbench/poller.c
create mode 100644 src/bin/pgbench/poller.h
create mode 100644 src/bin/pgbench/script.c
create mode 100644 src/bin/pgbench/script.h
create mode 100644 src/bin/pgbench/stats.c
create mode 100644 src/bin/pgbench/stats.h
create mode 100644 src/bin/pgbench/variable.c
create mode 100644 src/bin/pgbench/variable.h

--
2.56.0.rc1.315.gc6ed9934b7-goog

Zsolt Parragi

zsolt.parragi@percona.com

3 days ago

In reply to: Hannu Krosing (#1)

Re: [PATCH] Refactor pgbench to make future improvements easier

Hello!

From the email explaining that this is a split, and the commit messages saying "extract" I expected a move-only patchset that strictly extracts part of the code into different files/modules. But that doesn't seem to be the case?

For example "executeMetaCommand" looks completely different in the end, chooseScript and other functions got signature changes, the number of comment lines got reduces by ~25%, a VariableScopeStack typedef is introduced that isn't used anywhere, etc.

I think for something like this to be easily reviewable, moves and logic changes should be strictly separate patches, not mixed together, and anything that's not a simple cut-and-paste into another file should be mentioned. In the current patchset, 0007 seems to be the closest to a pure move, but even that isn't just that. (When I am doing something similar, I usually follow a one refactoring - one commit/patch approach during the review, only squashing things together later)

I also checked the commit history of pgbench, it seems to get around ~4 backpatched commits per year, so that doesn't seem to be that bad through a refactoring.

Hannu Krosing

hannu@tm.ee

2 days ago

In reply to: Zsolt Parragi (#2)

Re: [PATCH] Refactor pgbench to make future improvements easier

Hi Zsolt,

Thanks for reviewing this

Sorry for leaving more changes in than absolutely needed.

This was extracted back from a more invasive set of patches that

* extracts the various random distributions and other functions from
pgbench language and making them available as an in-database extension
https://commitfest.postgresql.org/patch/7225/

* adds a few more functions and language constructs to make it easier
to write TPC-* - like benchmarks

Would it make it easier for you to review if I reworked this set to
strictly move functions between source files, or are you still able to
review this as it is?

Best Regards

Hannu

Show quoted text

On Sun, Oct 4, 2026 at 12:20 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Hello!

From the email explaining that this is a split, and the commit
messages saying "extract" I expected a move-only patchset that
strictly extracts part of the code into different files/modules. But
that doesn't seem to be the case?

For example "executeMetaCommand" looks completely different in the
end, chooseScript and other functions got signature changes, the
number of comment lines got reduces by ~25%, a VariableScopeStack
typedef is introduced that isn't used anywhere, etc.

I think for something like this to be easily reviewable, moves and
logic changes should be strictly separate patches, not mixed together,
and anything that's not a simple cut-and-paste into another file
should be mentioned. In the current patchset, 0007 seems to be the
closest to a pure move, but even that isn't just that. (When I am
doing something similar, I usually follow a one refactoring - one
commit/patch approach during the review, only squashing things
together later)

I also checked the commit history of pgbench, it seems to get around
~4 backpatched commits per year, so that doesn't seem to be that bad
through a refactoring.

Hannu Krosing

hannu@tm.ee

2 days ago

In reply to: Hannu Krosing (#3)

Re: [PATCH] Refactor pgbench to make future improvements easier

The prliminary gap analysis for what is needed in pgbench to support
TPC and CH-benCHmark style workloads is in
https://wiki.postgresql.org/wiki/Pgbench-for-tpc-like-benchmarks

Show quoted text

On Sun, Oct 4, 2026 at 11:17 AM Hannu Krosing <hannuk@google.com> wrote:

Hi Zsolt,

Thanks for reviewing this

Sorry for leaving more changes in than absolutely needed.

This was extracted back from a more invasive set of patches that

* extracts the various random distributions and other functions from
pgbench language and making them available as an in-database extension
https://commitfest.postgresql.org/patch/7225/

* adds a few more functions and language constructs to make it easier
to write TPC-* - like benchmarks

Would it make it easier for you to review if I reworked this set to
strictly move functions between source files, or are you still able to
review this as it is?

Best Regards

Hannu

On Sun, Oct 4, 2026 at 12:20 AM Zsolt Parragi <zsolt.parragi@percona.com> wrote:

Hello!

From the email explaining that this is a split, and the commit
messages saying "extract" I expected a move-only patchset that
strictly extracts part of the code into different files/modules. But
that doesn't seem to be the case?

For example "executeMetaCommand" looks completely different in the
end, chooseScript and other functions got signature changes, the
number of comment lines got reduces by ~25%, a VariableScopeStack
typedef is introduced that isn't used anywhere, etc.

I think for something like this to be easily reviewable, moves and
logic changes should be strictly separate patches, not mixed together,
and anything that's not a simple cut-and-paste into another file
should be mentioned. In the current patchset, 0007 seems to be the
closest to a pure move, but even that isn't just that. (When I am
doing something similar, I usually follow a one refactoring - one
commit/patch approach during the review, only squashing things
together later)

I also checked the commit history of pgbench, it seems to get around
~4 backpatched commits per year, so that doesn't seem to be that bad
through a refactoring.

[PATCH] Refactor pgbench to make future improvements easier

Attachments: