Re: Extend pgbench partitioning to pgbench_history

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