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

From 2164409cb62f3dc3904822f56a8cc7cd3a4e4e18 Mon Sep 17 00:00:00 2001
From: Hannu Krosing <hannuk(at)google(dot)com>
Date: Sun, 11 Oct 2026 13:26:28 +0000
Subject: [PATCH v4 00/12] Modularize pgbench: pure code-movement extraction
(0001-0007) followed by API/encapsulation changes (0008-0012)

Hi Zolt, hackers,

This v4 patch series restructures the modularization of
`src/bin/pgbench/pgbench.c`
(~8,080 lines on `master` at `173cf85a079`) into two strictly separated
phases on
branch `pgbenc-refactor-obly`:

1. **Patches 0001-0007 (Pure Code Movement + Strictly Required Linkage
Adjustments)**:
Extracts `stats.c/h`, `variable.c/h`, `poller.c/h`, `script.c/h`,
`commands.c/h`,
`init.c/h`, and `engine.c/h` from `pgbench.c`.
- Zero function bodies, expressions, statements, parameter lists, return
types,
or comments are modified in patches 0001-0007.
- All 136 function definitions in `master:src/bin/pgbench/pgbench.c` (129
unique
functions, plus 7 `#ifdef`-paired definitions for the 6 `poller.c`
functions and
`setalarm`) are preserved verbatim at commit 0007.
- The **only** changes to any function or variable declaration in 0001-0007
are the
C linkage adjustments strictly required for multi-file compilation:
a) Removing the `static` storage-class specifier from the **54 unique
functions**
(61 definitions counting `#ifdef` branches) and the shared global variables
that are defined in one `.c` file and referenced from another `.c`
translation
unit, and declaring `extern` prototypes for them in the module headers. In C
(ISO/IEC 9899:1999 6.2.2), `static` gives an identifier internal linkage
visible only within a single translation unit; once caller and callee live
in
separate `.c` files, external linkage is mandatory or linking fails with
undefined symbol errors (and `-Wall -Wunused-function` warns in the defining
translation unit).
b) Moving the two `static inline` timing helpers (`pg_time_now` and
`pg_time_now_lazy`) verbatim into `stats.h`. Under C99 6.7.4, an `inline`
function with internal linkage (`static inline`) must have its definition
visible in every translation unit that calls it (`script.c`, `commands.c`,
`init.c`, `engine.c`, and `pgbench.c`).
c) Keeping `static` on all **69 internal helper functions** that are only
ever
called within their own `.c` file.

2. **Patches 0008-0012 (Functional, Encapsulation, and API Changes)**:
Applies the functional/API improvements in separate commits after the pure
extraction:
- **0008 (stats.c/h)**: Adds `mergeStats()` to deduplicate per-thread
`StatsData`
accumulation in `printProgressReport()` and `main()`, decouples `stats.c`
from
the global `throttle_delay` and `failures_detailed` variables via explicit
parameters on `accumStats()` and `getResultString()`, and adds `const`
qualifiers
to `mergeSimpleStats()` and `printSimpleStats()`.
- **0009 (variable.c/h)**: Upgrades `Variables` into a lexical scope stack
(`VariableScope` / `VariableScopeStack`), adds lifecycle and scope
management
functions (`initVariables()`, `destroyVariables()`, `copyVariables()`,
`var_scope_push()`, `var_scope_pop()`, `var_put_value()`,
`var_get_value()`),
exports `lookupCreateVariable()`, `is_an_int()`, and `valueTypeName()`, adds
`const` qualifiers, and replaces the manual startup variable copy loop in
`main()` with `copyVariables()`.
- **0010 (poller.c/h)**: Moves the platform `#ifdef`s (`POLL_USING_PPOLL` /
`POLL_USING_SELECT`) and `struct socket_set` definitions from `poller.h`
into
`poller.c` so `socket_set` is an opaque type, adds
`socket_wait_method_name()`,
and adds the `PgBenchPoller` typedef and `poller_*` macro aliases.
- **0011 (script.c/h, engine.c/h)**: Changes `chooseScript(TState *thread)`
to
`chooseScript(pg_prng_state *random_state)` so `script.h` no longer depends
on
`TState`.
- **0012 (commands.c/h, engine.c)**: Introduces `CommandHandler` and
`CommandDescriptor` (`command_handlers[]`), decomposes
`executeMetaCommand()`
into `handle_cmd_*` functions, and extracts `skipConditionalCommands()` and
`processGSetResult()` from `engine.c` into `commands.c/h`.

Changes in v4:
- Rebased onto latest `master` (`173cf85a079`).
- Added `#include <unistd.h>` to `src/bin/pgbench/init.c` (commit 0006) for
`isatty()`,
fixing the `macOS - Meson` build.

Hannu Krosing (12):
Refactor pgbench: extract statistics structures and functions to
stats.c/h
Refactor pgbench: extract variable management and value types to
variable.c/h
Refactor pgbench: extract socket polling abstraction to poller.c/h
Refactor pgbench: extract script parsing, built-in scripts, and
expression evaluation to script.c/h
Refactor pgbench: extract meta-command execution and prepared
statement helpers to commands.c/h
Refactor pgbench: extract database initialization (-i) to init.c/h
Refactor pgbench: extract benchmark execution engine and client state
machine to engine.c/h
Refactor pgbench stats: add mergeStats() and decouple global flags
Refactor pgbench variables: introduce scoped variable store
Refactor pgbench poller: encapsulate socket_set and add poller API
aliases
Refactor pgbench script: decouple chooseScript() from TState
Refactor pgbench commands: introduce pluggable meta-command dispatch

src/bin/pgbench/Makefile | 7 +
src/bin/pgbench/commands.c | 734 ++++
src/bin/pgbench/commands.h | 70 +
src/bin/pgbench/engine.c | 2159 +++++++++++
src/bin/pgbench/engine.h | 159 +
src/bin/pgbench/init.c | 828 ++++
src/bin/pgbench/init.h | 51 +
src/bin/pgbench/meson.build | 7 +
src/bin/pgbench/pgbench.c | 7167 +----------------------------------
src/bin/pgbench/pgbench.h | 193 +-
src/bin/pgbench/poller.c | 242 ++
src/bin/pgbench/poller.h | 46 +
src/bin/pgbench/script.c | 1946 ++++++++++
src/bin/pgbench/script.h | 170 +
src/bin/pgbench/stats.c | 199 +
src/bin/pgbench/stats.h | 164 +
src/bin/pgbench/variable.c | 842 ++++
src/bin/pgbench/variable.h | 128 +
18 files changed, 7953 insertions(+), 7159 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

base-commit: 173cf85a079516fdd2b09ed853702b05fbfcd15e
--
2.56.0.385.gd3acb90ef8-goog

On Sun, Oct 4, 2026 at 12:32 PM Hannu Krosing <hannuk(at)google(dot)com> wrote:

> 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
>
> On Sun, Oct 4, 2026 at 11:17 AM Hannu Krosing <hannuk(at)google(dot)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(dot)parragi(at)percona(dot)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.
>