Hello Amit,
I would not bother to create a patch for so small an improvement. This
makes sense in passing because the created function makes it very easy,
but otherwise I'll just drop it.
I would prefer to drop for now.
Attached v13 does that, I added a comment instead. I do not think that it
is an improvement.
+ else
+ {
+ fprintf(stderr, "unexpected partition method: \"%s\"\n", ps);
+ exit(1);
+ }
If we can't catch that earlier, then it might be better to have some
version-specific checks rather than such obscure code which is
difficult to understand for others.
Hmmm. The code simply checks for the current partitioning and fails if the
result is unknown, which I understood was what you asked, the previous
version was just ignoring the result.
The likelyhood of postgres dropping support for range or hash partitions
seems unlikely.
This issue rather be raised if an older partition-enabled pgbench is run
against a newer postgres which adds a new partition method. But then I
cannot guess when a new partition method will be added, so I cannot put a
guard with a version about something in the future. Possibly, if no new
method is ever added, the code will never be triggered.
I have made a few modifications in the attached patch.
* move the create partitions related code into a separate function.
Why not. Not sure it is an improvement.
* make the check related to number of partitions consistent i.e check
partitions > 0 apart from where we print which I also want to change
but let us first discuss one of the above points
I switched two instances of >= 1 to > 0, which had 1 instance before.
* when we don't found pgbench_accounts table, error out instead of continuing
I do not think that it is a a good idea, but I did it anyway to move
things forward.
* ensure append_fillfactor doesn't assume that it has to append
fillfactor and removed fillfactor < 100 check from it.
Done, which is too bad.
* improve the comments around query to fetch partitions
What? How?
There are already quite a few comments compared to the length of the
query.
* improve the comments in the patch and make the code look like nearby
code
This requirement is to fuzzy. I re-read the changes, and both code and
comments look okay to me.
* pgindent the patch
Done.
I think we should try to add some note or comment that why we only
choose to partition pgbench_accounts table when the user has given
--partitions option.
Added as a comment on the initPartition function.
--
Fabien.
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index c857aa3cba..e3a0abb4c7 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -306,6 +306,31 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry>
+ <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.
+ Default is <literal>0</literal>, meaning no partitioning.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term><option>--partition-method=<replaceable>NAME</replaceable></option></term>
+ <listitem>
+ <para>
+ Create a partitioned <literal>pgbench_accounts</literal> table with
+ <replaceable>NAME</replaceable> method.
+ Expected values are <literal>range</literal> or <literal>hash</literal>.
+ This option requires that <option>--partitions</option> is set to non-zero.
+ If unspecified, default is <literal>range</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
<varlistentry>
<term><option>--tablespace=<replaceable>tablespace</replaceable></option></term>
<listitem>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index ed7652bfbf..10eadd8e96 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -186,6 +186,19 @@ int64 latency_limit = 0;
char *tablespace = NULL;
char *index_tablespace = NULL;
+/* partitioning for pgbench_accounts table, 0 for no partitioning */
+static int partitions = 0;
+
+typedef enum
+{
+ PART_NONE, PART_RANGE, PART_HASH
+}
+
+ partition_method_t;
+
+static partition_method_t partition_method = PART_NONE;
+static const char *PARTITION_METHOD[] = {"none", "range", "hash"};
+
/* random seed used to initialize base_random_sequence */
int64 random_seed = -1;
@@ -617,6 +630,9 @@ usage(void)
" --foreign-keys create foreign key constraints between tables\n"
" --index-tablespace=TABLESPACE\n"
" create indexes in the specified tablespace\n"
+ " --partitions=NUM partition pgbench_accounts in NUM parts (default: 0)\n"
+ " --partition-method=(range|hash)\n"
+ " partition pgbench_accounts with this method (default: range)\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"
@@ -3601,6 +3617,80 @@ initDropTables(PGconn *con)
"pgbench_tellers");
}
+/*
+ * add fillfactor percent option.
+ */
+static void
+append_fillfactor(char *opts, int len)
+{
+ /* as default is 100, it could be removed in this case */
+ snprintf(opts + strlen(opts), len - strlen(opts),
+ " with (fillfactor=%d)", fillfactor);
+}
+
+/*
+ * Initialize pgbench_accounts partitions if needed.
+ *
+ * This is the larger table of pgbench default tpc-b like schema
+ * with a known size, so that it can be partitioned by range.
+ */
+static void
+initPartitions(PGconn *con)
+{
+ char ff[64];
+
+ ff[0] = '\0';
+ append_fillfactor(ff, sizeof(ff));
+
+ Assert(partitions > 0);
+
+ fprintf(stderr, "creating %d partitions...\n", partitions);
+
+ for (int p = 1; p <= partitions; p++)
+ {
+ char query[256];
+
+ if (partition_method == PART_RANGE)
+ {
+ int64 part_size = (naccounts * (int64) scale + partitions - 1) / partitions;
+ char minvalue[32],
+ maxvalue[32];
+
+ /*
+ * For RANGE, we use open-ended partitions at the beginning and
+ * end
+ */
+ if (p == 1)
+ sprintf(minvalue, "minvalue");
+ else
+ sprintf(minvalue, INT64_FORMAT, (p - 1) * part_size + 1);
+
+ if (p < partitions)
+ sprintf(maxvalue, INT64_FORMAT, p * part_size + 1);
+ else
+ sprintf(maxvalue, "maxvalue");
+
+ snprintf(query, sizeof(query),
+ "create%s table pgbench_accounts_%d\n"
+ " partition of pgbench_accounts\n"
+ " for values from (%s) to (%s)%s\n",
+ unlogged_tables ? " unlogged" : "", p,
+ minvalue, maxvalue, ff);
+ }
+ else if (partition_method == PART_HASH)
+ snprintf(query, sizeof(query),
+ "create%s table pgbench_accounts_%d\n"
+ " partition of pgbench_accounts\n"
+ " for values with (modulus %d, remainder %d)%s\n",
+ unlogged_tables ? " unlogged" : "", p,
+ partitions, p - 1, ff);
+ else /* cannot get there */
+ Assert(0);
+
+ executeStatement(con, query);
+ }
+}
+
/*
* Create pgbench's standard tables
*/
@@ -3664,9 +3754,15 @@ initCreateTables(PGconn *con)
/* Construct new create table statement. */
opts[0] = '\0';
- if (ddl->declare_fillfactor)
+
+ /* Partition pgbench_accounts table */
+ if (partitions > 0 && strcmp(ddl->table, "pgbench_accounts") == 0)
snprintf(opts + strlen(opts), sizeof(opts) - strlen(opts),
- " with (fillfactor=%d)", fillfactor);
+ " partition by %s (aid)", PARTITION_METHOD[partition_method]);
+ else if (ddl->declare_fillfactor)
+ /* fillfactor is only expected on actual tables */
+ append_fillfactor(opts, sizeof(opts));
+
if (tablespace != NULL)
{
char *escape_tablespace;
@@ -3686,6 +3782,9 @@ initCreateTables(PGconn *con)
executeStatement(con, buffer);
}
+
+ if (partitions > 0)
+ initPartitions(con);
}
/*
@@ -4919,6 +5018,10 @@ printResults(StatsData *total, instr_time total_time,
printf("transaction type: %s\n",
num_scripts == 1 ? sql_script[0].desc : "multiple scripts");
printf("scaling factor: %d\n", scale);
+ /* only print partitioning information if some partitioning was detected */
+ if (partition_method != PART_NONE)
+ printf("partition method: %s\npartitions: %d\n",
+ PARTITION_METHOD[partition_method], partitions);
printf("query mode: %s\n", QUERYMODE[querymode]);
printf("number of clients: %d\n", nclients);
printf("number of threads: %d\n", nthreads);
@@ -5126,6 +5229,8 @@ main(int argc, char **argv)
{"foreign-keys", no_argument, NULL, 8},
{"random-seed", required_argument, NULL, 9},
{"show-script", required_argument, NULL, 10},
+ {"partitions", required_argument, NULL, 11},
+ {"partition-method", required_argument, NULL, 12},
{NULL, 0, NULL, 0}
};
@@ -5486,6 +5591,29 @@ main(int argc, char **argv)
exit(0);
}
break;
+ case 11: /* partitions */
+ initialization_option_set = true;
+ partitions = atoi(optarg);
+ if (partitions < 0)
+ {
+ fprintf(stderr, "invalid number of partitions: \"%s\"\n",
+ optarg);
+ exit(1);
+ }
+ break;
+ case 12: /* partition-method */
+ initialization_option_set = true;
+ if (pg_strcasecmp(optarg, "range") == 0)
+ partition_method = PART_RANGE;
+ else if (pg_strcasecmp(optarg, "hash") == 0)
+ partition_method = PART_HASH;
+ else
+ {
+ fprintf(stderr, "invalid partition method, expecting \"range\" or \"hash\","
+ " got: \"%s\"\n", optarg);
+ exit(1);
+ }
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -5567,6 +5695,16 @@ main(int argc, char **argv)
exit(1);
}
+ if (partitions == 0 && partition_method != PART_NONE)
+ {
+ fprintf(stderr, "--partition-method requires greater than zero --partitions\n");
+ exit(1);
+ }
+
+ /* set default method */
+ if (partitions > 0 && partition_method == PART_NONE)
+ partition_method = PART_RANGE;
+
if (initialize_steps == NULL)
initialize_steps = pg_strdup(DEFAULT_INIT_STEPS);
@@ -5756,6 +5894,70 @@ main(int argc, char **argv)
fprintf(stderr,
"scale option ignored, using count from pgbench_branches table (%d)\n",
scale);
+
+ /*
+ * Gather partition information from pg_catalog.
+ *
+ * We Assume no partitioning on any failure, so as to avoid failing on
+ * an older version.
+ */
+ res = PQexec(con,
+ "select o.n, p.partstrat, pg_catalog.count(i.inhparent) "
+ /* for all tables */
+ "from pg_catalog.pg_class as c "
+ /* get the schema corresponding to the previous table */
+ "join pg_catalog.pg_namespace as n on (n.oid = c.relnamespace) "
+ /* get this schema order in search_path */
+ "cross join lateral (select pg_catalog.array_position(pg_catalog.current_schemas(true), n.nspname)) as o(n) "
+ /* check whether it is partitionned */
+ "left join pg_catalog.pg_partitioned_table as p on (p.partrelid = c.oid) "
+ /* fetch actual partitions which inherits the main table */
+ "left join pg_catalog.pg_inherits as i on (c.oid = i.inhparent) "
+ /* check table name and that schema was in search_path */
+ "where c.relname = 'pgbench_accounts' and o.n is not null "
+ /* count partitions, possibly 0 */
+ "group by 1, 2 "
+ /* and only keep the first encountered */
+ "order by 1 asc "
+ "limit 1");
+ if (PQresultStatus(res) != PGRES_TUPLES_OK)
+ {
+ /* probably an older version, coldly assume no partitioning */
+ partition_method = PART_NONE;
+ partitions = 0;
+ }
+ else if (PQntuples(res) == 0)
+ {
+ /* builtin script would fail later anyway... */
+ fprintf(stderr, "No pgbench_accounts table found in search_path\n");
+ exit(1);
+ }
+ else
+ {
+ /* PQntupes(res) == 1: normal case, extract the partition status */
+ if (PQgetisnull(res, 0, 1))
+ partition_method = PART_NONE;
+ else
+ {
+ char *ps = PQgetvalue(res, 0, 1);
+
+ Assert(ps != NULL);
+
+ if (strcmp(ps, "r") == 0)
+ partition_method = PART_RANGE;
+ else if (strcmp(ps, "h") == 0)
+ partition_method = PART_HASH;
+ else
+ {
+ fprintf(stderr, "unexpected partition method: \"%s\"\n", ps);
+ exit(1);
+ }
+ }
+
+ partitions = atoi(PQgetvalue(res, 0, 2));
+ }
+
+ PQclear(res);
}
/*
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index b82d3f65c4..fb0f6b677d 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -58,6 +58,17 @@ sub pgbench
return;
}
+# tablespace for testing
+my $ts = $node->basedir . '/regress_pgbench_tap_1_ts_dir';
+mkdir $ts or die "cannot create directory $ts";
+my $ets = TestLib::perl2host($ts);
+# add needed escaping!
+$ets =~ s/'/''/;
+
+$node->safe_psql('postgres',
+ "CREATE TABLESPACE regress_pgbench_tap_1_ts LOCATION '$ets';"
+);
+
# Test concurrent OID generation via pg_enum_oid_index. This indirectly
# exercises LWLock and spinlock concurrency.
my $labels = join ',', map { "'l$_'" } 1 .. 1000;
@@ -100,12 +111,13 @@ pgbench(
# Again, with all possible options
pgbench(
- '--initialize --init-steps=dtpvg --scale=1 --unlogged-tables --fillfactor=98 --foreign-keys --quiet --tablespace=pg_default --index-tablespace=pg_default',
+ '--initialize --init-steps=dtpvg --scale=1 --unlogged-tables --fillfactor=98 --foreign-keys --quiet --tablespace=regress_pgbench_tap_1_ts --index-tablespace=regress_pgbench_tap_1_ts --partitions=2 --partition-method=hash',
0,
[qr{^$}i],
[
qr{dropping old tables},
qr{creating tables},
+ qr{creating 2 partitions},
qr{vacuuming},
qr{creating primary keys},
qr{creating foreign keys},
@@ -116,12 +128,13 @@ pgbench(
# Test interaction of --init-steps with legacy step-selection options
pgbench(
- '--initialize --init-steps=dtpvgvv --no-vacuum --foreign-keys --unlogged-tables',
+ '--initialize --init-steps=dtpvgvv --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
0,
[qr{^$}],
[
qr{dropping old tables},
qr{creating tables},
+ qr{creating 3 partitions},
qr{creating primary keys},
qr{.* of .* tuples \(.*\) done},
qr{creating foreign keys},
@@ -909,6 +922,8 @@ pgbench(
check_pgbench_logs($bdir, '001_pgbench_log_3', 1, 10, 10,
qr{^\d \d{1,2} \d+ \d \d+ \d+$});
+$node->safe_psql('postgres', 'DROP TABLESPACE regress_pgbench_tap_1_ts');
+
# done
$node->stop;
done_testing();
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index f7fa18418b..1e9542af3f 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -157,6 +157,13 @@ my @options = (
qr{error while setting random seed from --random-seed option}
]
],
+ [ 'bad partition type', '-i --partition-method=BAD', [qr{"range"}, qr{"hash"}, qr{"BAD"}] ],
+ [ 'bad partition number', '-i --partitions -1', [ qr{invalid number of partitions: "-1"} ] ],
+ [
+ 'partition method without partitioning',
+ '-i --partition-method=hash',
+ [ qr{partition-method requires greater than zero --partitions} ]
+ ],
# logging sub-options
[
