Hello Masao-san,
I do not think that this is desirable. It would be a regression, and
allowing a no-op is not an issue in anyway.
Why is that regression, you think?
Because "pgbench -I ' d'" currently works and it would cease to work after
the patch.
I think that's an oversight. If I'm missing something and accepting a
blank character as no-op in also checkInitSteps() is really necessary
for some reasons, which should be documented. But, if so, another
question is; why should only blank character be treated as no-op, in
checkInitSteps()?
The idea is to have one character that can be substituted to remove any
operation.
On principle, allowing a no-op character, whatever the choice, is a good
idea, because it means that the caller can take advantage of that if need
be.
I think that the actual oversight is that the checkInitSteps should be
called at the beginning of processing initialization steps rather than
while processing -I, because currently other places modify the
initialization string (no-vacuum, foreign key) and thus are not checked.
I agree that it should be documented.
Attached patch adds a doc and moves the check where it should be, and
modifies a test with an explicit no-op space initialization step.
--
Fabien.
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 4c48a58ed2..5008377998 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -168,7 +168,8 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
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 available steps are:
+ The space character is accepted as a no-op, and the other available
+ steps are:
<variablelist>
<varlistentry>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 14dbc4510c..4178127c21 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -4104,6 +4104,9 @@ runInitSteps(const char *initialize_steps)
double run_time = 0.0;
bool first = true;
+ /* check that all steps are valid before executing them */
+ checkInitSteps(initialize_steps);
+
initPQExpBuffer(&stats);
if ((con = doConnect()) == NULL)
@@ -5501,7 +5504,6 @@ main(int argc, char **argv)
if (initialize_steps)
pg_free(initialize_steps);
initialize_steps = pg_strdup(optarg);
- checkInitSteps(initialize_steps);
initialization_option_set = true;
break;
case 'h':
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 1845869016..4e3d3e464c 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -130,7 +130,7 @@ pgbench(
# Test interaction of --init-steps with legacy step-selection options
pgbench(
- '--initialize --init-steps=dtpvGvv --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
+ '--initialize --no-vacuum --foreign-keys --unlogged-tables --partitions=3',
0,
[qr{^$}],
[
@@ -143,7 +143,9 @@ pgbench(
qr{(?!vacuuming)}, # no vacuum
qr{done in \d+\.\d\d s }
],
- 'pgbench --init-steps');
+ 'pgbench --init-steps',
+ undef,
+ '--init-steps= dtpvGvv');
# Run all builtin scripts, for a few transactions each
pgbench(
