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

Hi Zsolt

I have now extracted only the pure mo
Subject: [PATCH v7 00/10]

Hi Zsolt, hackers,

I have now moved the fast-paced do-everything development into a
separate github repository.

But as I still think refactoring the single-file pgbench.c into
functional modules is useful for anyone wanting to improve pgbench, so
here is a patch set to do just that.

I also split it into a few more modules than the original patch set.

I plan to follow up with patches to extract the user-callable
functions and make them available in both pgbench app and inside
PostgreSQL as the pgbench extension.

Following the discussion on splitting pgbench.c into focused modules before
layering any API, encapsulation, or feature changes on top, this v7 series
is scoped strictly to pure mechanical code and data movement across 10
focused patches (0001-0010):

0001: stats.c / stats.h (statistics structures, timing
helpers, accumulators)
0002: variable.c / variable.h (PgBenchValue, client variables, type coercion)
0003: poller.c / poller.h (ppoll(2) / select(2) socket-set abstraction)
0004: random.c / random.h (PRNG state, distributions,
FNV-1a/Murmur2, permute)
0005: expr.c / expr.h (recursive expression AST & built-in
function evaluator)
0006: script.c / script.h (SQL/backslash script parser &
built-in TPC-B scripts)
0007: commands.c / commands.h (shell runner, prepared helpers,
\sleep, meta-commands)
0008: init.c / init.h (database initialization -i DDL & data
population)
0009: engine.c / engine.h (connections,
advanceConnectionState(), threadRun(), alarms)
0010: report.c / report.h (doLog(), processXactStats(),
progress, printResults())

After patch 0010, src/bin/pgbench/pgbench.c is reduced from 8,069 lines to
931 lines containing only CLI option globals, usage(), and main().

Strict Mechanical Movement Invariants (0001-0010):
--------------------------------------------------
1. Zero changes to function bodies or signatures:
Every one of the 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) has an identical body, parameter list, return type,
and const qualification at every commit 0001 through 0010.

2. Minimal linkage adjustments required by C99 for multi-TU compilation:
- The `static` specifier is removed ONLY on the 66 unique functions (73
definitions counting #ifdef pairs) and shared global variables that are
defined in one .c file and referenced from another .c translation unit,
with matching `extern` declarations in the corresponding module .h file.
- All 57 single-translation-unit helper definitions (plus the 2 `static
inline` timing helpers pg_time_now() and pg_time_now_lazy() placed
verbatim in stats.h) retain their `static` linkage.
- The 4 functions that were already non-static in master (strtoint64,
strtodouble, syntax_error, and main) keep their existing external linkage.

3. Monotonic, zero-reordering diffs in pgbench.c:
No remaining code in pgbench.c is reordered at any step, so each patch's
diff against pgbench.c consists solely of `#include` updates, `static`
removals on cross-TU symbols, and contiguous block deletions.

Changes in v7:
--------------
- Rebased onto master (77705d1e5a6).
- Isolated the pure mechanical code-movement extraction into its own
standalone 10-patch series (0001-0010), with zero functional, signature,
const-qualification, or encapsulation changes mixed in.
- Split the former monolithic script/expression/random extraction patch into
three smaller, single-responsibility patches:
* 0004 (random.c/h): PRNG state, uniform/exponential/Gaussian/Poisson/Zipfian
distributions, FNV-1a and Murmur2 hashes, and permute().
* 0005 (expr.c/h): recursive expression AST and built-in function evaluator.
* 0006 (script.c/h): SQL and backslash meta-command script parsing and
built-in script definitions.
- Split reporting and transaction logging out of the engine extraction patch
into patch 0010 (report.c/h: doLog(), processXactStats(),
printProgressReport(), printVersion(), and printResults()).

On Sat, Oct 24, 2026 at 6:36 AM Zsolt Parragi <zsolt(dot)parragi(at)percona(dot)com> wrote:
>
> On Fri, 23 Oct 2026, Koshi Shibagaki (Fujitsu)
> <shibagaki(dot)koshi(at)fujitsu(dot)com> wrote:
> > For v6, patches 0001-0007 seem like a suitable scope for that review.
> > I have left 0008-0012 out of this scope because they go beyond code
> > movement and include changes to internal interfaces and structures.
> > Those changes could be discussed separately, along with the new
> > features.
>
> +1
>
> As I promised at pgconfeu, I tried to take another look at 1-7 later
> to see if they are properly code-movement commits.
>
> But generally I would suggest the same (and what was also suggested in
> the other thread[1]): put the idea, "overall PoC" part on github or
> another git hosting, resending the the entire patchset every time
> doesn't help.
>
> A simple link and summary would be enough in this thread for anybody
> interested in it to take a look at it.
>
> I am sure people could argue about just the first 7 patches, where to
> put which function and such, and constantly rebasing and resending 27
> patches doesn't seem to be that useful to me. It also seems unlikely
> for somebody to commit this in one go, so from this perspective,
> keeping a single commitfest entry for everything is also wrong.
>
> That could also help you to focus on the quality of the first patches.
>
> One thing I immediately noticed is that the summary of 0004 is strange:
>
> src/bin/pgbench/Makefile | 1 +
> src/bin/pgbench/meson.build | 1 +
> src/bin/pgbench/pgbench.c | 6419 +++++++++++------------------------
> src/bin/pgbench/pgbench.h | 253 +-
> src/bin/pgbench/script.c | 1946 +++++++++++
> src/bin/pgbench/script.h | 170 +
> 6 files changed, 4434 insertions(+), 4356 deletions(-)
>
> If we are only moving code out from pgbench.c, why do we see thousands
> of added lines to it? So far I only took a quick glance at it, but
> seems like it still mixes things together: internal reorganization
> (such as commandFailed is now at a different place in the file), and
> moving code out of it.
>
> The reorganization part isn't mentioned anywhere in the commit message:
>
> Move the script/command data structures (MetaCommand, QueryMode, Command,
> ParsedScript, BuiltinScript), PRNG/distribution/hashing/permutation
> functions, expression evaluator, script selector, and SQL/backslash
> script parser out of pgbench.c into script.c and script.h. Also move the
> shared client/thread state types (ConnectionStateEnum, CState, TStatus,
> TState) into pgbench.h so both pgbench.c and script.c can access them.
>
> I would probably argue that this part of the really long commit
> message is already two things and should be two or more separate
> patches to reduce the size (script.c itself is 2000 lines), and it
> clearly doesn't mention "reorganizing pgbench.c" anywhere.
>
> And it shouldn't, I still think one patchfile should focus on a single
> well defined change, that can be described in a few sentences.
>
> Alternatively, since we are talking about simple reorganization, maybe
> it shouldn't even start with patches? Maybe a table (CSV?) of what
> functions to move where, and/or some text based diagram format like
> mermaid to visualize the architecture/structure you want to get, if
> you want to talk about the long term vision?
>
> A properly written pitch/design document could be an easier way for
> people to argue about it, and then it could help solve the question of
> how something this large can get committed?
>
> [1]: https://www.postgresql.org/message-id/atwDmsIb9TYfg9uH%40paquier.xyz