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

Thanks for detailed explanation.

I will start with a high level reorganizing and refactoring plan and a
diagram illustrating what I want to achieve.

And maybe even before that a more detailed description of the current
state to make the plan and the end goals easier to to discuss.

And until we have the ability in commitfest to link patches I will
provide instructions on how to get the actual pghackers-style patches
from my github if it gets to that point.

------------
Hannu

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