Add table access method as an option to pgbench
Hi Hackers,
I noticed that there is a table access method API added starting from
PG12. In other words, Postgresql open the door for developers to add
their own access methods, for example, zheap, zedstore etc. However, the
current pgbench doesn't have an option to allow user to specify which
table access method to use during the initialization phase. If we can
have an option to help user address this issue, it would be great. For
example, if user want to do a performance benchmark to compare "zheap"
with "heap", then the commands can be something like below:
pgbench -d -i postgres --table-am=heap
pgbench -d -i postgres --table-am=zheap
I know there is a parameter like below available in postgresql.conf:
#default_table_access_method = 'heap'
But, providing another option for the end user may not be a bad idea,
and it might make the tests easier at some points.
The attached file is quick patch for this.
Thoughts?
Thank you,
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
0001-add-table-access-method-option-to-pgbench.patchtext/plain; charset=UTF-8; name=0001-add-table-access-method-option-to-pgbench.patch; x-mac-creator=0; x-mac-type=0Download
From 798f970e22034d33146265ed98922c605d0dc237 Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Tue, 24 Nov 2020 15:14:42 -0800
Subject: [PATCH] add table access method option to pgbench
---
src/bin/pgbench/pgbench.c | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 08a5947a9e..24bc6bdbe3 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -188,6 +188,11 @@ int64 latency_limit = 0;
char *tablespace = NULL;
char *index_tablespace = NULL;
+/*
+ * table access method selection
+ */
+char *tableam = NULL;
+
/*
* Number of "pgbench_accounts" partitions. 0 is the default and means no
* partitioning.
@@ -643,6 +648,7 @@ usage(void)
" --partitions=NUM partition pgbench_accounts into NUM parts (default: 0)\n"
" --tablespace=TABLESPACE create tables in the specified tablespace\n"
" --unlogged-tables create tables as unlogged tables\n"
+ " --table-am=TABLEAM create tables using specified access method\n"
"\nOptions to select what to run:\n"
" -b, --builtin=NAME[@W] add builtin script NAME weighted at W (default: 1)\n"
" (use \"-b list\" to list available scripts)\n"
@@ -3750,12 +3756,14 @@ initCreateTables(PGconn *con)
for (i = 0; i < lengthof(DDLs); i++)
{
char opts[256];
+ char opam[256];
char buffer[256];
const struct ddlinfo *ddl = &DDLs[i];
const char *cols;
/* Construct new create table statement. */
opts[0] = '\0';
+ opam[0] = '\0';
/* Partition pgbench_accounts table */
if (partition_method != PART_NONE && strcmp(ddl->table, "pgbench_accounts") == 0)
@@ -3776,11 +3784,22 @@ initCreateTables(PGconn *con)
PQfreemem(escape_tablespace);
}
+ if (tableam != NULL)
+ {
+ char *escape_tableam;
+
+ escape_tableam = PQescapeIdentifier(con, tableam,
+ strlen(tableam));
+ snprintf(opam + strlen(opam), sizeof(opam) - strlen(opam),
+ " using %s", escape_tableam);
+ PQfreemem(escape_tableam);
+ }
+
cols = (scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols;
- snprintf(buffer, sizeof(buffer), "create%s table %s(%s)%s",
+ snprintf(buffer, sizeof(buffer), "create%s table %s(%s)%s%s",
unlogged_tables ? " unlogged" : "",
- ddl->table, cols, opts);
+ ddl->table, cols, opam, opts);
executeStatement(con, buffer);
}
@@ -5422,6 +5441,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"table-am", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -5795,6 +5815,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13: /* table-am */
+ initialization_option_set = true;
+ tableam = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
--
2.17.1
On Tue, Nov 24, 2020 at 03:32:38PM -0800, David Zhang wrote:
But, providing another option for the end user may not be a bad idea, and it
might make the tests easier at some points.
My first thought is that we have no need to complicate pgbench with
this option because there is a GUC able to do that, but we do that for
tablespaces, so... No objections from here.
The attached file is quick patch for this.
Thoughts?
This patch does not apply on HEAD, where you can just use
appendPQExpBuffer() to append the new clause to the CREATE TABLE
query. This needs proper documentation.
--
Michael
Thank a lot for your comments, Michael.
On 2020-11-24 7:41 p.m., Michael Paquier wrote:
On Tue, Nov 24, 2020 at 03:32:38PM -0800, David Zhang wrote:
But, providing another option for the end user may not be a bad idea, and it
might make the tests easier at some points.My first thought is that we have no need to complicate pgbench with
this option because there is a GUC able to do that, but we do that for
tablespaces, so... No objections from here.The attached file is quick patch for this.
Thoughts?
This patch does not apply on HEAD, where you can just use
appendPQExpBuffer() to append the new clause to the CREATE TABLE
query. This needs proper documentation.
The previous patch was based on branch "REL_13_STABLE". Now, the
attached new patch v2 is based on master branch. I followed the new code
structure using appendPQExpBuffer to append the new clause "using
TABLEAM" in a proper position and tested. In the meantime, I also
updated the pgbench.sqml file to reflect the changes.
--
Michael
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
v2-0001-add-table-access-method-option-to-pgbench.patchtext/plain; charset=UTF-8; name=v2-0001-add-table-access-method-option-to-pgbench.patch; x-mac-creator=0; x-mac-type=0Download
From 1e908ecdd6add81c96dca489f45f5ae9d0c2a5f1 Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Wed, 25 Nov 2020 10:41:20 -0800
Subject: [PATCH] add table access method option to pgbench
---
doc/src/sgml/ref/pgbench.sgml | 11 +++++++++++
src/bin/pgbench/pgbench.c | 20 ++++++++++++++++++++
2 files changed, 31 insertions(+)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 7180fedd65..a8f8d24fe1 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -378,6 +378,17 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--table-am=<replaceable>TABLEAM</replaceable></option></term>
+ <listitem>
+ <para>
+ Create all tables with specified table access method
+ <replaceable>TABLEAM</replaceable>.
+ If unspecified, default is <literal>heap</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</para>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 3057665bbe..62b1dd3e23 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -189,6 +189,11 @@ int64 latency_limit = 0;
char *tablespace = NULL;
char *index_tablespace = NULL;
+/*
+ * table access method selection
+ */
+char *tableam = NULL;
+
/*
* Number of "pgbench_accounts" partitions. 0 is the default and means no
* partitioning.
@@ -643,6 +648,7 @@ usage(void)
" --partitions=NUM partition pgbench_accounts into NUM parts (default: 0)\n"
" --tablespace=TABLESPACE create tables in the specified tablespace\n"
" --unlogged-tables create tables as unlogged tables\n"
+ " --table-am=TABLEAM create tables with specified table access method (default: heap)\n"
"\nOptions to select what to run:\n"
" -b, --builtin=NAME[@W] add builtin script NAME weighted at W (default: 1)\n"
" (use \"-b list\" to list available scripts)\n"
@@ -3759,6 +3765,15 @@ initCreateTables(PGconn *con)
ddl->table,
(scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
+ if (tableam != NULL)
+ {
+ char *escape_tableam;
+
+ escape_tableam = PQescapeIdentifier(con, tableam, strlen(tableam));
+ appendPQExpBuffer(&query, " using %s", escape_tableam);
+ PQfreemem(escape_tableam);
+ }
+
/* Partition pgbench_accounts table */
if (partition_method != PART_NONE && strcmp(ddl->table, "pgbench_accounts") == 0)
appendPQExpBuffer(&query,
@@ -5419,6 +5434,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"table-am", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -5792,6 +5808,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13: /* table access method*/
+ initialization_option_set = true;
+ tableam = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
--
2.17.1
On Wed, Nov 25, 2020 at 12:13:55PM -0800, David Zhang wrote:
The previous patch was based on branch "REL_13_STABLE". Now, the attached
new patch v2 is based on master branch. I followed the new code structure
using appendPQExpBuffer to append the new clause "using TABLEAM" in a proper
position and tested. In the meantime, I also updated the pgbench.sqml file
to reflect the changes.
+ <varlistentry>
+ <term><option>--table-am=<replaceable>TABLEAM</replaceable></option></term>
+ <listitem>
+ <para>
+ Create all tables with specified table access method
+ <replaceable>TABLEAM</replaceable>.
+ If unspecified, default is <literal>heap</literal>.
+ </para>
+ </listitem>
+ </varlistentry>
This needs to be in alphabetical order. And what you say here is
wrong. The default would not be heap if default_table_access_method
is set to something else. I would suggest to use table_access_method
instead of TABLEAM, and keep the paragraph minimal, say:
"Create tables using the specified table access method, rather than
the default."--table-am is really the best option name? --table-access-method
sounds a bit more consistent to me.
+ if (tableam != NULL)
+ {
+ char *escape_tableam;
+
+ escape_tableam = PQescapeIdentifier(con, tableam, strlen(tableam));
+ appendPQExpBuffer(&query, " using %s", escape_tableam);
+ PQfreemem(escape_tableam);
+ }
The order is wrong here, generating an unsupported grammar, see by
yourself with this command:
pgbench --partition-method=hash --table-am=heap -i --partitions 2This makes the patch trickier than it looks as the USING clause is
located between PARTITION BY and WITH. Also, partitioned tables
cannot use the USING clause so you need to be careful that
createPartitions() also uses the correct table AM.
This stuff needs tests.
--
Michael
Hello David,
The previous patch was based on branch "REL_13_STABLE". Now, the attached new
patch v2 is based on master branch. I followed the new code structure using
appendPQExpBuffer to append the new clause "using TABLEAM" in a proper
position and tested. In the meantime, I also updated the pgbench.sqml file to
reflect the changes.
My 0.02�: I'm fine with the added feature.
The patch lacks minimal coverage test. Consider adding something to
pgbench tap tests, including failures (ie non existing method).
The option in the help string is not at the right ab place.
I would merge the tableam declaration to the previous one with a extended
comments, eg "tablespace and access method selection".
escape_tableam -> escaped_tableam ?
--
Fabien.
Thanks a lot again for the review and comments.
On 2020-11-25 9:36 p.m., Michael Paquier wrote:
On Wed, Nov 25, 2020 at 12:13:55PM -0800, David Zhang wrote:
The previous patch was based on branch "REL_13_STABLE". Now, the attached
new patch v2 is based on master branch. I followed the new code structure
using appendPQExpBuffer to append the new clause "using TABLEAM" in a proper
position and tested. In the meantime, I also updated the pgbench.sqml file
to reflect the changes.+ <varlistentry> + <term><option>--table-am=<replaceable>TABLEAM</replaceable></option></term> + <listitem> + <para> + Create all tables with specified table access method + <replaceable>TABLEAM</replaceable>. + If unspecified, default is <literal>heap</literal>. + </para> + </listitem> + </varlistentry> This needs to be in alphabetical order.
The order issue is fixed in v3 patch.
And what you say here is
wrong. The default would not be heap if default_table_access_method
is set to something else.
Right, if user change the default settings in GUC, then the default is
not `heap` any more.
I would suggest to use table_access_method
instead of TABLEAM,
All other options if values are required, the words are all capitalized,
such as TABLESPACE. Therefore, I used TABLEACCESSMETHOD in stead of
table_access_method in v3 patch.
and keep the paragraph minimal, say:
"Create tables using the specified table access method, rather than
the default."
Updated in v3 patch.
--table-am is really the best option name? --table-access-method
sounds a bit more consistent to me.
Updated in v3 patch.
+ if (tableam != NULL) + { + char *escape_tableam; + + escape_tableam = PQescapeIdentifier(con, tableam, strlen(tableam)); + appendPQExpBuffer(&query, " using %s", escape_tableam); + PQfreemem(escape_tableam); + }
Thanks for pointing this out. The order I believe is fixed in v3 patch.
The order is wrong here, generating an unsupported grammar, see by
yourself with this command:
pgbench --partition-method=hash --table-am=heap -i --partitions 2
Tested in v3 patch, with a command like,
pgbench --partition-method=hash --table-access-method=heap -i --partitions 2
This makes the patch trickier than it looks as the USING clause is
located between PARTITION BY and WITH. Also, partitioned tables
cannot use the USING clause so you need to be careful that
createPartitions() also uses the correct table AM.This stuff needs tests.
3 test cases has been added the tap test, but honestly I am not sure if
I am following the rules. Any comments will be great.
--
Michael
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
v3-0001-add-table-access-method-as-an-option-to-pgbench.patchtext/plain; charset=UTF-8; name=v3-0001-add-table-access-method-as-an-option-to-pgbench.patch; x-mac-creator=0; x-mac-type=0Download
From c64173842a163abbdd6210af5f9643b3301d8de0 Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Thu, 26 Nov 2020 18:08:49 -0800
Subject: [PATCH] add table access method as an option to pgbench
---
doc/src/sgml/ref/pgbench.sgml | 9 ++++++
src/bin/pgbench/pgbench.c | 23 ++++++++++++++-
src/bin/pgbench/t/001_pgbench_with_server.pl | 30 +++++++++++++++++++-
3 files changed, 60 insertions(+), 2 deletions(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 7180fedd65..3af5230f85 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -359,6 +359,15 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--table-access-method=<replaceable>TABLEACCESSMETHOD</replaceable></option></term>
+ <listitem>
+ <para>
+ Create tables using the specified table access method, rather than the default.
+ </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 3057665bbe..c3939b6bb4 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -184,10 +184,11 @@ double throttle_delay = 0;
int64 latency_limit = 0;
/*
- * tablespace selection
+ * tablespace and table access method selection
*/
char *tablespace = NULL;
char *index_tablespace = NULL;
+char *table_access_method = NULL;
/*
* Number of "pgbench_accounts" partitions. 0 is the default and means no
@@ -641,6 +642,9 @@ usage(void)
" --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"
+ " --table-access-method=TABLEACCESSMETHOD\n"
+ " create tables with using specified table access method, \n"
+ " rather than the default.\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"
@@ -3759,6 +3763,18 @@ initCreateTables(PGconn *con)
ddl->table,
(scale >= SCALE_32BIT_THRESHOLD) ? ddl->bigcols : ddl->smcols);
+ if (partition_method == PART_NONE && table_access_method != NULL)
+ {
+ char *escaped_table_access_method;
+
+ escaped_table_access_method =
+ PQescapeIdentifier(con,
+ table_access_method, strlen(table_access_method));
+ appendPQExpBuffer(&query, " using %s",
+ escaped_table_access_method);
+ PQfreemem(escaped_table_access_method);
+ }
+
/* Partition pgbench_accounts table */
if (partition_method != PART_NONE && strcmp(ddl->table, "pgbench_accounts") == 0)
appendPQExpBuffer(&query,
@@ -5419,6 +5435,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"table-access-method", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -5792,6 +5809,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13: /* table access method*/
+ initialization_option_set = true;
+ table_access_method = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 61b671d54f..4b780fd0e0 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -114,7 +114,7 @@ pgbench(
# Again, with all possible options
pgbench(
- '--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',
+ '--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 --table-access-method=heap',
0,
[qr{^$}i],
[
@@ -129,6 +129,22 @@ pgbench(
],
'pgbench scale 1 initialization');
+# Again, with all possible options except partition
+pgbench(
+ '--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 --table-access-method=heap',
+ 0,
+ [qr{^$}i],
+ [
+ qr{dropping old tables},
+ qr{creating tables},
+ qr{vacuuming},
+ qr{creating primary keys},
+ qr{creating foreign keys},
+ qr{(?!vacuuming)}, # no vacuum
+ qr{done in \d+\.\d\d s }
+ ],
+ 'pgbench scale 1 initialization');
+
# Test interaction of --init-steps with legacy step-selection options
pgbench(
'--initialize --init-steps=dtpvGvv --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
@@ -146,6 +162,18 @@ pgbench(
],
'pgbench --init-steps');
+# Test interaction of --table-access
+pgbench(
+ '--initialize --table-access-method=heap',
+ 0,
+ [qr{^$}],
+ [
+ qr{dropping old tables},
+ qr{creating tables},
+ qr{done in \d+\.\d\d s }
+ ],
+ 'pgbench --table-access');
+
# Run all builtin scripts, for a few transactions each
pgbench(
'--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t'
--
2.17.1
Thanks Fabien for the comments.
On 2020-11-25 11:29 p.m., Fabien COELHO wrote:
Hello David,
The previous patch was based on branch "REL_13_STABLE". Now, the
attached new patch v2 is based on master branch. I followed the new
code structure using appendPQExpBuffer to append the new clause
"using TABLEAM" in a proper position and tested. In the meantime, I
also updated the pgbench.sqml file to reflect the changes.My 0.02€: I'm fine with the added feature.
The patch lacks minimal coverage test. Consider adding something to
pgbench tap tests, including failures (ie non existing method).
I added 3 test cases to the tap tests, but not sure if they are
following the rules. One question about the failures test, my thoughts
is that it should be in the no_server test cases, but it is hard to
verify the input as the table access method can be any name, such as
zheap, zedstore or zheap2 etc. Any suggestion will be great.
The option in the help string is not at the right ab place.
Fixed in v3 patch.
I would merge the tableam declaration to the previous one with a
extended comments, eg "tablespace and access method selection".
Updated in v3 patch.
escape_tableam -> escaped_tableam ?
Updated in v3 patch.
By the way, I saw the same style for other variables, such as
escape_tablespace, should this be fixed as well?
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Hello David,
Some feedback about v3:
In the doc I find TABLEACCESSMETHOD quite hard to read. Use TABLEAM
instead? Also, the next entry uses lowercase (tablespace), why change the
style?
Remove space after comma in help string. I'd use the more readable TABLEAM
in the help string rather than the mouthful.
It looks that the option is *silently* ignored when creating a
partitionned table, which currently results in an error, and not passed to
partitions, which would accept them. This is pretty weird.
I'd suggest that the table am option should rather by changing the default
instead, so that it would apply to everything relevant implicitely when
appropriate.
About tests :
They should also trigger failures, eg
"--table-access-method=no-such-table-am", to be added to the @errors list.
I do not understand why you duplicated all possible option entry.
Test with just table access method looks redundant if the feature is make
to work orthonogonally to partitions, which should be the case.
By the way, I saw the same style for other variables, such as
escape_tablespace, should this be fixed as well?
Nope, let it be.
--
Fabien.
Thanks a lot for the comments, Fabien.
Some feedback about v3:
In the doc I find TABLEACCESSMETHOD quite hard to read. Use TABLEAM
instead?
Yes, in this case, *TABLEAM* is easy to read, updated.
Also, the next entry uses lowercase (tablespace), why change the style?
The original style is not so consistent, for example, in doc,
--partition-method using *NAME*, but --table-space using *tablespace*;
in help, --partition-method using *(rang|hash)*, but --tablespace using
*TABLESPACE*. To keep it more consistent, I would rather use *TABLEAM*
in both doc and help.
Remove space after comma in help string. I'd use the more readable
TABLEAM in the help string rather than the mouthful.
Yes the *space* is removed after comma.
It looks that the option is *silently* ignored when creating a
partitionned table, which currently results in an error, and not
passed to partitions, which would accept them. This is pretty weird.
The input check is added with an error message when both partitions and
table access method are used.
I'd suggest that the table am option should rather by changing the
default instead, so that it would apply to everything relevant
implicitely when appropriate.
I think this is a better idea, so in v4 patch I change it to something
like "set default_table_access_method to *TABLEAM*" instead of using the
*using* clause.
About tests :
They should also trigger failures, eg
"--table-access-method=no-such-table-am", to be added to the @errors
list.
No sure how to address this properly, since the table access method
potentially can be *any* name.
I do not understand why you duplicated all possible option entry.
Test with just table access method looks redundant if the feature is
make to work orthonogonally to partitions, which should be the case.
Only one positive test case added using *heap* as the table access
method to verify the initialization.
By the way, I saw the same style for other variables, such as
escape_tablespace, should this be fixed as well?Nope, let it be.
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
v4-0001-add-table-access-method-as-an-option-to-pgbench.patchtext/plain; charset=UTF-8; name=v4-0001-add-table-access-method-as-an-option-to-pgbench.patch; x-mac-creator=0; x-mac-type=0Download
From c25b55580b1b3f3faac63b4d72291c80fb2c9c1f Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Tue, 1 Dec 2020 20:47:01 -0800
Subject: [PATCH] add table access method as an option to pgbench
---
doc/src/sgml/ref/pgbench.sgml | 10 +++++++
src/bin/pgbench/pgbench.c | 31 +++++++++++++++++++-
src/bin/pgbench/t/001_pgbench_with_server.pl | 12 ++++++++
3 files changed, 52 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 7180fedd65..992fbbdb4b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -359,6 +359,16 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--table-access-method=<replaceable>TABLEAM</replaceable></option></term>
+ <listitem>
+ <para>
+ Create tables using the specified table access method, rather than the default.
+ tablespace.
+ </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 3057665bbe..961841b255 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -184,10 +184,11 @@ double throttle_delay = 0;
int64 latency_limit = 0;
/*
- * tablespace selection
+ * tablespace and table access method selection
*/
char *tablespace = NULL;
char *index_tablespace = NULL;
+char *table_access_method = NULL;
/*
* Number of "pgbench_accounts" partitions. 0 is the default and means no
@@ -641,6 +642,9 @@ usage(void)
" --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"
+ " --table-access-method=TABLEAM\n"
+ " create tables with using specified table access method,\n"
+ " rather than the default.\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"
@@ -3747,6 +3751,20 @@ initCreateTables(PGconn *con)
fprintf(stderr, "creating tables...\n");
+ if (table_access_method != NULL && partition_method == PART_NONE && partitions == 0)
+ {
+ char *escaped_table_access_method;
+
+ initPQExpBuffer(&query);
+ escaped_table_access_method = PQescapeIdentifier(con,
+ table_access_method, strlen(table_access_method));
+ appendPQExpBuffer(&query, "set default_table_access_method to %s;\n",
+ escaped_table_access_method);
+ PQfreemem(escaped_table_access_method);
+ executeStatement(con, query.data);
+ termPQExpBuffer(&query);
+ }
+
initPQExpBuffer(&query);
for (i = 0; i < lengthof(DDLs); i++)
@@ -5419,6 +5437,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"table-access-method", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -5792,6 +5811,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13: /* table access method*/
+ initialization_option_set = true;
+ table_access_method = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
@@ -5907,6 +5930,12 @@ main(int argc, char **argv)
}
}
+ if (table_access_method != NULL && (partitions > 0 || partition_method != PART_NONE))
+ {
+ pg_log_fatal("--table-access-method cannot work with --partition-method or --partitions");
+ exit(1);
+ }
+
runInitSteps(initialize_steps);
exit(0);
}
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 61b671d54f..c81b1ac17a 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -146,6 +146,18 @@ pgbench(
],
'pgbench --init-steps');
+# Test interaction of --table-access
+pgbench(
+ '--initialize --table-access-method=heap',
+ 0,
+ [qr{^$}],
+ [
+ qr{dropping old tables},
+ qr{creating tables},
+ qr{done in \d+\.\d\d s }
+ ],
+ 'pgbench --table-access');
+
# Run all builtin scripts, for a few transactions each
pgbench(
'--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t'
--
2.17.1
Hello David,
Some feedback about v4.
It looks that the option is *silently* ignored when creating a partitionned
table, which currently results in an error, and not passed to partitions,
which would accept them. This is pretty weird.The input check is added with an error message when both partitions and table
access method are used.
Hmmm. If you take the resetting the default, I do not think that you
should have to test anything? AFAICT the access method is valid on
partitions, although not on the partitioned table declaration. So I'd say
that you could remove the check.
They should also trigger failures, eg
"--table-access-method=no-such-table-am", to be added to the @errors list.No sure how to address this properly, since the table access method
potentially can be *any* name.
I think it is pretty unlikely that someone would chose the name
"no-such-table-am" when developing a new storage engine for Postgres
inside Postgres, so that it would make this internal test fail.
There are numerous such names used in tests, eg "no-such-database",
"no-such-command".
So I'd suggest to add such a test to check for the expected failure.
I do not understand why you duplicated all possible option entry.
Test with just table access method looks redundant if the feature is make
to work orthonogonally to partitions, which should be the case.Only one positive test case added using *heap* as the table access method to
verify the initialization.
Yep, only "heap" if currently available for tables.
--
Fabien.
Again, thanks a lot for the feedback.
On 2020-12-02 12:03 p.m., Fabien COELHO wrote:
Hello David,
Some feedback about v4.
It looks that the option is *silently* ignored when creating a
partitionned table, which currently results in an error, and not
passed to partitions, which would accept them. This is pretty weird.The input check is added with an error message when both partitions
and table access method are used.Hmmm. If you take the resetting the default, I do not think that you
should have to test anything? AFAICT the access method is valid on
partitions, although not on the partitioned table declaration. So I'd
say that you could remove the check.
Yes, it makes sense to remove the *blocking* check, and actually the
table access method interface does work with partitioned table. I tested
this/v5 by duplicating the heap access method with a different name. For
this reason, I removed the condition check as well when applying the
table access method.
They should also trigger failures, eg
"--table-access-method=no-such-table-am", to be added to the @errors
list.No sure how to address this properly, since the table access method
potentially can be *any* name.I think it is pretty unlikely that someone would chose the name
"no-such-table-am" when developing a new storage engine for Postgres
inside Postgres, so that it would make this internal test fail.There are numerous such names used in tests, eg "no-such-database",
"no-such-command".So I'd suggest to add such a test to check for the expected failure.
The test case "no-such-table-am" has been added to the errors list, and
the regression test is ok.
I do not understand why you duplicated all possible option entry.
Test with just table access method looks redundant if the feature is
make to work orthonogonally to partitions, which should be the case.Only one positive test case added using *heap* as the table access
method to verify the initialization.Yep, only "heap" if currently available for tables.
Thanks,
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
v5-0001-add-table-access-method-as-an-option-to-pgbench.patchtext/plain; charset=UTF-8; name=v5-0001-add-table-access-method-as-an-option-to-pgbench.patch; x-mac-creator=0; x-mac-type=0Download
From 1d6f434d56c36f95da82d1bae4f99bb917351c08 Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Mon, 7 Dec 2020 13:42:00 -0800
Subject: [PATCH] add table access method as an option to pgbench
---
doc/src/sgml/ref/pgbench.sgml | 10 ++++++++
src/bin/pgbench/pgbench.c | 25 +++++++++++++++++++-
src/bin/pgbench/t/001_pgbench_with_server.pl | 22 +++++++++++++++++
3 files changed, 56 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 7180fedd65..992fbbdb4b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -359,6 +359,16 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--table-access-method=<replaceable>TABLEAM</replaceable></option></term>
+ <listitem>
+ <para>
+ Create tables using the specified table access method, rather than the default.
+ tablespace.
+ </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 3057665bbe..eb91df6d6b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -184,10 +184,11 @@ double throttle_delay = 0;
int64 latency_limit = 0;
/*
- * tablespace selection
+ * tablespace and table access method selection
*/
char *tablespace = NULL;
char *index_tablespace = NULL;
+char *table_access_method = NULL;
/*
* Number of "pgbench_accounts" partitions. 0 is the default and means no
@@ -641,6 +642,9 @@ usage(void)
" --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"
+ " --table-access-method=TABLEAM\n"
+ " create tables with using specified table access method,\n"
+ " rather than the default.\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"
@@ -3747,6 +3751,20 @@ initCreateTables(PGconn *con)
fprintf(stderr, "creating tables...\n");
+ if (table_access_method != NULL)
+ {
+ char *escaped_table_access_method;
+
+ initPQExpBuffer(&query);
+ escaped_table_access_method = PQescapeIdentifier(con,
+ table_access_method, strlen(table_access_method));
+ appendPQExpBuffer(&query, "set default_table_access_method to %s;\n",
+ escaped_table_access_method);
+ PQfreemem(escaped_table_access_method);
+ executeStatement(con, query.data);
+ termPQExpBuffer(&query);
+ }
+
initPQExpBuffer(&query);
for (i = 0; i < lengthof(DDLs); i++)
@@ -5419,6 +5437,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"table-access-method", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -5792,6 +5811,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13: /* table access method*/
+ initialization_option_set = true;
+ table_access_method = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 61b671d54f..077e862530 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -100,6 +100,16 @@ pgbench(
[qr{Perhaps you need to do initialization}],
'run without init');
+pgbench(
+ '-i --table-access-method=no-such-table-am',
+ 1,
+ [qr{^$}],
+ [
+ qr{invalid value for parameter "default_table_access_method"},
+ qr{DETAIL: Table access method "no-such-table-am" does not exist}
+ ],
+ 'no such table access method');
+
# Initialize pgbench tables scale 1
pgbench(
'-i', 0,
@@ -146,6 +156,18 @@ pgbench(
],
'pgbench --init-steps');
+# Test interaction of --table-access-method
+pgbench(
+ '--initialize --table-access-method=heap',
+ 0,
+ [qr{^$}],
+ [
+ qr{dropping old tables},
+ qr{creating tables},
+ qr{done in \d+\.\d\d s }
+ ],
+ 'pgbench --table-access-method');
+
# Run all builtin scripts, for a few transactions each
pgbench(
'--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t'
--
2.17.1
--- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -359,6 +359,16 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d </listitem> </varlistentry>+ <varlistentry> + <term><option>--table-access-method=<replaceable>TABLEAM</replaceable></option></term> + <listitem> + <para> + Create tables using the specified table access method, rather than the default. + tablespace.
tablespace is an extraneous word ?
Also, I mention that the regression tests do this to get a 2nd AM:
src/test/regress/sql/create_am.sql:CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
...
src/test/regress/sql/create_am.sql:DROP ACCESS METHOD heap2;
David:
If you found pre-existing inconsistencies/errors/deficiencies, I'd suggest to
fix them in a separate patch. Typically those are small, and you can make them
0001 patch. Errors might be worth backpatching, so in addition to making the
"feature" patches small, it's good if they're separate.
Like writing NAME vs TABLESPACE. Or escape vs escapes.
Or maybe using SET default_tablespace instead of modifying the CREATE sql.
That'd need to be done separately for indexes, and RESET after creating the
tables, to avoid accidentally affecting indexes, too.
@Fabien: I think the table should be created and populated within the same
transaction, to allow this optimization in v13 to apply during the
"initialization" phase.
c6b92041d Skip WAL for new relfilenodes, under wal_level=minimal.
--
Justin
Hello Justin,
src/test/regress/sql/create_am.sql:CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
...
src/test/regress/sql/create_am.sql:DROP ACCESS METHOD heap2;
Or maybe using SET default_tablespace instead of modifying the CREATE sql.
That'd need to be done separately for indexes, and RESET after creating the
tables, to avoid accidentally affecting indexes, too.
Why should it not affect indexes?
@Fabien: I think the table should be created and populated within the same
transaction, to allow this optimization in v13 to apply during the
"initialization" phase.
c6b92041d Skip WAL for new relfilenodes, under wal_level=minimal.
Possibly.
This would be a change of behavior: currently only filling tables is under
an explicit transaction.
That would be a matter for another patch, probably with an added option.
As VACUUM is not transaction compatible, it might be a little bit tricky
to add such a feature. Also I'm not sure about some constraint
declarations.
ISTM that I submitted a patch a long time ago to allow "()" to control
transaction from the command line, but somehow it got lost or rejected.
I redeveloped it, see attached. I cannot see reliable performance
improvement by playing with (), though.
--
Fabien.
Attachments:
pgbench-init-tx-1.patchtext/x-diff; name=pgbench-init-tx-1.patchDownload
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index b03d0cc50f..8d9b642fdd 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -170,7 +170,7 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
<replaceable>init_steps</replaceable> specifies the
initialization steps to be performed, using one character per step.
Each step is invoked in the specified order.
- The default is <literal>dtgvp</literal>.
+ The default is <literal>dt(g)vp</literal>.
The available steps are:
<variablelist>
@@ -226,6 +226,22 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</para>
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><literal>(</literal> (Begin)</term>
+ <listitem>
+ <para>
+ Begin a transaction.
+ </para>
+ </listitem>
+ </varlistentry>
+ <varlistentry>
+ <term><literal>)</literal> (Commit)</term>
+ <listitem>
+ <para>
+ Commit a transaction.
+ </para>
+ </listitem>
+ </varlistentry>
<varlistentry>
<term><literal>v</literal> (Vacuum)</term>
<listitem>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 3057665bbe..f77e33056c 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -132,8 +132,8 @@ static int pthread_join(pthread_t th, void **thread_return);
/********************************************************************
* some configurable parameters */
-#define DEFAULT_INIT_STEPS "dtgvp" /* default -I setting */
-#define ALL_INIT_STEPS "dtgGvpf" /* all possible steps */
+#define DEFAULT_INIT_STEPS "dt(g)vp" /* default -I setting */
+#define ALL_INIT_STEPS "dtgGvpf()" /* all possible steps */
#define LOG_STEP_SECONDS 5 /* seconds between log messages */
#define DEFAULT_NXACTS 10 /* default nxacts */
@@ -3823,12 +3823,6 @@ initGenerateDataClientSide(PGconn *con)
fprintf(stderr, "generating data (client-side)...\n");
- /*
- * we do all of this in one transaction to enable the backend's
- * data-loading optimizations
- */
- executeStatement(con, "begin");
-
/* truncate away any old data */
initTruncateTables(con);
@@ -3940,8 +3934,6 @@ initGenerateDataClientSide(PGconn *con)
}
termPQExpBuffer(&sql);
-
- executeStatement(con, "commit");
}
/*
@@ -3958,12 +3950,6 @@ initGenerateDataServerSide(PGconn *con)
fprintf(stderr, "generating data (server-side)...\n");
- /*
- * we do all of this in one transaction to enable the backend's
- * data-loading optimizations
- */
- executeStatement(con, "begin");
-
/* truncate away any old data */
initTruncateTables(con);
@@ -3989,8 +3975,6 @@ initGenerateDataServerSide(PGconn *con)
executeStatement(con, sql.data);
termPQExpBuffer(&sql);
-
- executeStatement(con, "commit");
}
/*
@@ -4076,6 +4060,8 @@ initCreateFKeys(PGconn *con)
static void
checkInitSteps(const char *initialize_steps)
{
+ bool in_tx = false;
+
if (initialize_steps[0] == '\0')
{
pg_log_fatal("no initialization steps specified");
@@ -4090,6 +4076,36 @@ checkInitSteps(const char *initialize_steps)
pg_log_info("Allowed step characters are: \"" ALL_INIT_STEPS "\".");
exit(1);
}
+
+ if (*step == '(')
+ {
+ if (in_tx)
+ {
+ pg_log_fatal("opening a transaction inside a transaction");
+ exit(1);
+ }
+ in_tx = true;
+ }
+ else if (*step == ')')
+ {
+ if (!in_tx)
+ {
+ pg_log_fatal("closing a transaction without opening it");
+ exit(1);
+ }
+ in_tx = false;
+ }
+ else if (*step == 'v' && in_tx)
+ {
+ pg_log_fatal("cannot run vacuum within a transaction");
+ exit(1);
+ }
+ }
+
+ if (in_tx)
+ {
+ pg_log_fatal("uncommitted transaction");
+ exit(1);
}
}
@@ -4150,6 +4166,14 @@ runInitSteps(const char *initialize_steps)
op = "foreign keys";
initCreateFKeys(con);
break;
+ case '(':
+ op = "begin";
+ executeStatement(con, "begin");
+ break;
+ case ')':
+ op = "commit";
+ executeStatement(con, "commit");
+ break;
case ' ':
break; /* ignore */
default:
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index e38c7d77d1..11c677ae54 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -139,6 +139,8 @@ my @options = (
'--progress-timestamp => --progress', '--progress-timestamp',
[qr{allowed only under}]
],
+
+ # initialization
[
'-I without init option',
'-I dtg',
@@ -152,6 +154,26 @@ my @options = (
qr{Allowed step characters are}
]
],
+ [
+ 'bad init tx 1',
+ '-i -I )',
+ [ qr{open} ]
+ ],
+ [
+ 'bad init tx 2',
+ '-i -I (',
+ [ qr{uncommit} ]
+ ],
+ [
+ 'bad init tx 3',
+ '-i -I (v)',
+ [ qr{vacuum} ]
+ ],
+ [
+ 'bad init tx 4',
+ '-i -I ((',
+ [ qr{inside} ]
+ ],
[
'bad random seed',
'--random-seed=one',
On Sun, Dec 27, 2020 at 09:14:53AM -0400, Fabien COELHO wrote:
src/test/regress/sql/create_am.sql:CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
...
src/test/regress/sql/create_am.sql:DROP ACCESS METHOD heap2;Or maybe using SET default_tablespace instead of modifying the CREATE sql.
That'd need to be done separately for indexes, and RESET after creating the
tables, to avoid accidentally affecting indexes, too.Why should it not affect indexes?
I rarely use pgbench, and probably never looked at its source before, but I saw
that it has a separate --tablespace and --index-tablespace, and that
--tablespace is *not* the default for indexes.
So if we changed it to use SET default_tablespace instead of amending the DDL
sql, we'd need to make sure the SET applied only to the intended CREATE
command, and not all following create commands. In the case that
--index-tablespace is not specified, it would be buggy to do this:
SET default_tablespace=foo;
CREATE TABLE ...
CREATE INDEX ...
CREATE TABLE ...
CREATE INDEX ...
...
--
Justin
PS. Thanks for patching it to work with partitioned tables :)
tablespace is an extraneous word ?
Thanks a lot for pointing this out. I will fix it in next patch once get all issues clarified.
On Sun, Dec 27, 2020 at 09:14:53AM -0400, Fabien COELHO wrote:
src/test/regress/sql/create_am.sql:CREATE ACCESS METHOD heap2 TYPE TABLE HANDLER heap_tableam_handler;
...
src/test/regress/sql/create_am.sql:DROP ACCESS METHOD heap2;
Do you suggest to add a new positive test case by using table access method *heap2* created using the existing heap_tableam_handler?
If you found pre-existing inconsistencies/errors/deficiencies, I'd suggest to
fix them in a separate patch. Typically those are small, and you can make them
0001 patch. Errors might be worth backpatching, so in addition to making the
"feature" patches small, it's good if they're separate.
Like writing NAME vs TABLESPACE. Or escape vs escapes.
I will provide a separate small patch when fixing the *tablespace* typo mention above. Can you help to clarity which releases or branches need to be back patched?
Or maybe using SET default_tablespace instead of modifying the CREATE sql.
That'd need to be done separately for indexes, and RESET after creating the
tables, to avoid accidentally affecting indexes, too.Why should it not affect indexes?
I rarely use pgbench, and probably never looked at its source before, but I saw
that it has a separate --tablespace and --index-tablespace, and that
--tablespace is *not* the default for indexes.So if we changed it to use SET default_tablespace instead of amending the DDL
sql, we'd need to make sure the SET applied only to the intended CREATE
command, and not all following create commands. In the case that
--index-tablespace is not specified, it would be buggy to do this:SET default_tablespace=foo;
CREATE TABLE ...
CREATE INDEX ...
CREATE TABLE ...
CREATE INDEX ...
...
If this `tablespace` issue also applies to `table-access-method`, yes, I agree it could be buggy too. But, the purpose of this patch is to provide an option for end user to test a newly created table access method. If the the *new* table access method hasn't fully implemented all the interfaces yet, then no matter the end user use the option provided by this patch or the GUC parameter, the results will be the same.
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
+ " create tables with using specified table access method,\n"
In my opinion, this sentence should be "create tables with specified table access method" or "create tables using specified table access method".
"create tables with specified table access method" may be more consistent with other options.
On 2021-01-09 5:44 a.m., youichi aramaki wrote:
+ " create tables with using specified table access method,\n"
In my opinion, this sentence should be "create tables with specified table access method" or "create tables using specified table access method".
"create tables with specified table access method" may be more consistent with other options.
Thank you Youichi. I will change it to "create tables with specified
table access method" in next patch.
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Hi,
`v6-0001-add-table-access-method-as-an-option-to-pgbench` fixed the
wording problems for pgbench document and help as they were pointed out
by Justin and Youichi.
`0001-update-tablespace-to-keep-document-consistency` is trying to make
the *tablespace* to be more consistent in pgbench document.
Thank you,
On 2021-01-14 3:51 p.m., David Zhang wrote:
On 2021-01-09 5:44 a.m., youichi aramaki wrote:
+ " create tables with using specified table access
method,\n"In my opinion, this sentence should be "create tables with specified
table access method" or "create tables using specified table access
method".
"create tables with specified table access method" may be more
consistent with other options.Thank you Youichi. I will change it to "create tables with specified
table access method" in next patch.
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca
Attachments:
0001-update-tablespace-to-keep-document-consistency.patchtext/plain; charset=UTF-8; name=0001-update-tablespace-to-keep-document-consistency.patch; x-mac-creator=0; x-mac-type=0Download
From ff94675fda38a0847b893a29dce8c3068a74e118 Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Fri, 15 Jan 2021 11:40:30 -0800
Subject: [PATCH] update tablespace to keep document consistency
---
doc/src/sgml/ref/pgbench.sgml | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index faa7c26b0a..080c25731d 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -325,10 +325,10 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</varlistentry>
<varlistentry>
- <term><option>--index-tablespace=<replaceable>index_tablespace</replaceable></option></term>
+ <term><option>--index-tablespace=<replaceable>TABLESPACE</replaceable></option></term>
<listitem>
<para>
- Create indexes in the specified tablespace, rather than the default
+ Create indexes in the specified <replaceable>TABLESPACE</replaceable>, rather than the default
tablespace.
</para>
</listitem>
@@ -360,10 +360,10 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</varlistentry>
<varlistentry>
- <term><option>--tablespace=<replaceable>tablespace</replaceable></option></term>
+ <term><option>--tablespace=<replaceable>TABLESPACE</replaceable></option></term>
<listitem>
<para>
- Create tables in the specified tablespace, rather than the default
+ Create tables in the specified <replaceable>TABLESPACE</replaceable>, rather than the default
tablespace.
</para>
</listitem>
--
2.17.1
v6-0001-add-table-access-method-as-an-option-to-pgbench.patchtext/plain; charset=UTF-8; name=v6-0001-add-table-access-method-as-an-option-to-pgbench.patch; x-mac-creator=0; x-mac-type=0Download
From 517fcb61c7065b7bccf6105a7bb89007fe2fcb08 Mon Sep 17 00:00:00 2001
From: David Zhang <david.zhang@highgo.ca>
Date: Fri, 15 Jan 2021 10:32:53 -0800
Subject: [PATCH] add table access method as an option to pgbench
---
doc/src/sgml/ref/pgbench.sgml | 9 +++++++
src/bin/pgbench/pgbench.c | 25 +++++++++++++++++++-
src/bin/pgbench/t/001_pgbench_with_server.pl | 22 +++++++++++++++++
3 files changed, 55 insertions(+), 1 deletion(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index faa7c26b0a..d8018388e6 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -359,6 +359,15 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</listitem>
</varlistentry>
+ <varlistentry>
+ <term><option>--table-access-method=<replaceable>TABLEAM</replaceable></option></term>
+ <listitem>
+ <para>
+ Create tables with specified table access method, rather than the default.
+ </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 f7da3e1f62..3f8a119811 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -184,10 +184,11 @@ double throttle_delay = 0;
int64 latency_limit = 0;
/*
- * tablespace selection
+ * tablespace and table access method selection
*/
char *tablespace = NULL;
char *index_tablespace = NULL;
+char *table_access_method = NULL;
/*
* Number of "pgbench_accounts" partitions. 0 is the default and means no
@@ -641,6 +642,9 @@ usage(void)
" --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"
+ " --table-access-method=TABLEAM\n"
+ " create tables with specified table access method,\n"
+ " rather than the default.\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"
@@ -3761,6 +3765,20 @@ initCreateTables(PGconn *con)
fprintf(stderr, "creating tables...\n");
+ if (table_access_method != NULL)
+ {
+ char *escaped_table_access_method;
+
+ initPQExpBuffer(&query);
+ escaped_table_access_method = PQescapeIdentifier(con,
+ table_access_method, strlen(table_access_method));
+ appendPQExpBuffer(&query, "set default_table_access_method to %s;\n",
+ escaped_table_access_method);
+ PQfreemem(escaped_table_access_method);
+ executeStatement(con, query.data);
+ termPQExpBuffer(&query);
+ }
+
initPQExpBuffer(&query);
for (i = 0; i < lengthof(DDLs); i++)
@@ -5433,6 +5451,7 @@ main(int argc, char **argv)
{"show-script", required_argument, NULL, 10},
{"partitions", required_argument, NULL, 11},
{"partition-method", required_argument, NULL, 12},
+ {"table-access-method", required_argument, NULL, 13},
{NULL, 0, NULL, 0}
};
@@ -5806,6 +5825,10 @@ main(int argc, char **argv)
exit(1);
}
break;
+ case 13: /* table access method*/
+ initialization_option_set = true;
+ table_access_method = pg_strdup(optarg);
+ break;
default:
fprintf(stderr, _("Try \"%s --help\" for more information.\n"), progname);
exit(1);
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 61b671d54f..077e862530 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -100,6 +100,16 @@ pgbench(
[qr{Perhaps you need to do initialization}],
'run without init');
+pgbench(
+ '-i --table-access-method=no-such-table-am',
+ 1,
+ [qr{^$}],
+ [
+ qr{invalid value for parameter "default_table_access_method"},
+ qr{DETAIL: Table access method "no-such-table-am" does not exist}
+ ],
+ 'no such table access method');
+
# Initialize pgbench tables scale 1
pgbench(
'-i', 0,
@@ -146,6 +156,18 @@ pgbench(
],
'pgbench --init-steps');
+# Test interaction of --table-access-method
+pgbench(
+ '--initialize --table-access-method=heap',
+ 0,
+ [qr{^$}],
+ [
+ qr{dropping old tables},
+ qr{creating tables},
+ qr{done in \d+\.\d\d s }
+ ],
+ 'pgbench --table-access-method');
+
# Run all builtin scripts, for a few transactions each
pgbench(
'--transactions=5 -Dfoo=bla --client=2 --protocol=simple --builtin=t'
--
2.17.1
Hi,
On 2020-11-25 12:41:25 +0900, Michael Paquier wrote:
On Tue, Nov 24, 2020 at 03:32:38PM -0800, David Zhang wrote:
But, providing another option for the end user may not be a bad idea, and it
might make the tests easier at some points.My first thought is that we have no need to complicate pgbench with
this option because there is a GUC able to do that, but we do that for
tablespaces, so... No objections from here.
I think that objection is right. All that's needed to change this from
the client side is to do something like
PGOPTIONS='-c default_table_access_method=foo' pgbench ...
I don't think adding pgbench options for individual GUCs really is a
useful exercise?
Greetings,
Andres Freund
On Fri, Jan 15, 2021 at 01:22:45PM -0800, Andres Freund wrote:
I think that objection is right. All that's needed to change this from
the client side is to do something like
PGOPTIONS='-c default_table_access_method=foo' pgbench ...I don't think adding pgbench options for individual GUCs really is a
useful exercise?
Yeah. Looking at the latest patch, it just uses SET
default_table_access_method to achieve its goal and to bypass the fact
that partitions don't support directly the AM clause. So I agree to
mark this patch as rejected and move on. One thing that looks like an
issue to me is that PGOPTIONS is not listed in the section for
environment variables on the docs of pgbench. 5aaa584 has plugged in
a lot of holes, but things could be improved more, for more clients,
where it makes sense of course.
--
Michael
On 2021-01-15 1:22 p.m., Andres Freund wrote:
Hi,
On 2020-11-25 12:41:25 +0900, Michael Paquier wrote:
On Tue, Nov 24, 2020 at 03:32:38PM -0800, David Zhang wrote:
But, providing another option for the end user may not be a bad idea, and it
might make the tests easier at some points.My first thought is that we have no need to complicate pgbench with
this option because there is a GUC able to do that, but we do that for
tablespaces, so... No objections from here.I think that objection is right. All that's needed to change this from
the client side is to do something like
PGOPTIONS='-c default_table_access_method=foo' pgbench ...
Yeah, this is a better solution for me too. Thanks a lot for all the
feedback.
I don't think adding pgbench options for individual GUCs really is a
useful exercise?Greetings,
Andres Freund
--
David
Software Engineer
Highgo Software Inc. (Canada)
www.highgo.ca