Re: Extend pgbench partitioning to pgbench_history

Hi Abhijit,

Thanks for your input. Please accept my updated patch.

Ciao,
Gabriele

On Tue, 16 Jan 2024 at 12:53, Abhijit Menon-Sen <ams(at)toroid(dot)org> wrote:

> At 2023-11-30 11:29:15 +0100, gabriele(dot)bartolini(at)enterprisedb(dot)com wrote:
> >
> > With the attached patch, I extend the partitioning capability to the
> > pgbench_history table too.
> >
> > I have been thinking of adding an option to control this, but I preferred
> > to ask in this list whether it really makes sense or not (I struggle
> indeed
> > to see use cases where accounts is partitioned and history is not).
>
> I don't have a strong opinion about this, but I also can't think of a
> reason to want to create partitions for pgbench_accounts but leave out
> pgbench_history.
>
> > From ba8f507b126a9c5bd22dd40bb8ce0c1f0c43ac59 Mon Sep 17 00:00:00 2001
> > From: Gabriele Bartolini <gabriele(dot)bartolini(at)enterprisedb(dot)com>
> > Date: Thu, 30 Nov 2023 11:02:39 +0100
> > Subject: [PATCH] Include pgbench_history in partitioning method for
> pgbench
> >
> > In case partitioning, make sure that pgbench_history is also partitioned
> with
> > the same criteria.
>
> I think "If partitioning" or "If we're creating partitions" would read
> better here. Also, same criteria as what? Maybe you could just add "as
> pgbench_accounts" to the end.
>
> > diff --git a/doc/src/sgml/ref/pgbench.sgml
> b/doc/src/sgml/ref/pgbench.sgml
> > index 05d3f81619..4c02d2a61d 100644
> > --- a/doc/src/sgml/ref/pgbench.sgml
> > +++ b/doc/src/sgml/ref/pgbench.sgml
> > […]
> > @@ -378,9 +378,9 @@ pgbench <optional>
> <replaceable>options</replaceable> </optional> <replaceable>d
> >
> <term><option>--partitions=<replaceable>NUM</replaceable></option></term>
> > <listitem>
> > <para>
> > - Create a partitioned <literal>pgbench_accounts</literal> table
> with
> > - <replaceable>NUM</replaceable> partitions of nearly equal size
> for
> > - the scaled number of accounts.
> > + Create partitioned <literal>pgbench_accounts</literal> and
> <literal>pgbench_history</literal>
> > + tables with <replaceable>NUM</replaceable> partitions of nearly
> equal size for
> > + the scaled number of accounts - and future history records.
> > Default is <literal>0</literal>, meaning no partitioning.
> > </para>
>
> I would just leave out the "-" and write "number of accounts and future
> history records".
>
> > diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
> > index 2e1650d0ad..87adaf4d8f 100644
> > --- a/src/bin/pgbench/pgbench.c
> > +++ b/src/bin/pgbench/pgbench.c
> > […]
> > @@ -889,8 +889,10 @@ usage(void)
> > " --index-tablespace=TABLESPACE\n"
> > " create indexes in the
> specified tablespace\n"
> > " --partition-method=(range|hash)\n"
> > - " partition pgbench_accounts
> with this method (default: range)\n"
> > - " --partitions=NUM partition pgbench_accounts
> into NUM parts (default: 0)\n"
> > + " partition pgbench_accounts
> and pgbench_history with this method"
> > + " (default: range)."
> > + " --partitions=NUM partition pgbench_accounts
> and pgbench_history into NUM parts"
> > + " (default: 0)\n"
> > " --tablespace=TABLESPACE create tables in the
> specified tablespace\n"
> > " --unlogged-tables create tables as unlogged
> tables\n"
> > "\nOptions to select what to run:\n"
>
> There's a missing newline after "(default: range).".
>
> I read through the rest of the patch closely. It looks fine to me. It
> applies, builds, and does create the partitions as intended.
>
> -- Abhijit
>

--
Gabriele Bartolini
Vice President, Cloud Native at EDB
enterprisedb.com