Re: Extend pgbench partitioning to pgbench_history

At 2023-11-30 11:29:15 +0100, gabriele.bartolini@enterprisedb.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.bartolini@enterprisedb.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