Avoiding inadvertent debugging mode for pgbench
Attached please find a patch to adjust the behavior of the pgbench program
and make it behave like the other programs that connect to a database
(namely, psql and pg_dump). Specifically, add support for using -d and
--dbname to specify the name of the database. This means that -d can no
longer be used to turn on debugging mode, and the long option --debug must
be used instead.
This removes a long-standing footgun, in which people assume that the -d
option behaves the same as other programs. Indeed, because it takes no
arguments, and because the first non-option argument is the database name,
it still appears to work. However, people then wonder why pgbench is so
darn verbose all the time! :)
This is a breaking change, but fixing it this way seems to have the least
total impact, as the number of people using the debug mode of pgbench is
likely quite small. Further, those already using the long option are
unaffected, and those using the short one simply need to replace '-d' with
'--debug', arguably making their scripts a little more self-documenting in
the process.
Cheers,
Greg
Attachments:
pgbench.dash.d.or.not.dash.d.v1.patchapplication/octet-stream; name=pgbench.dash.d.or.not.dash.d.v1.patchDownload
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 850028557d..ea6f49304e 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -461,7 +461,6 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</varlistentry>
<varlistentry id="pgbench-option-debug">
- <term><option>-d</option></term>
<term><option>--debug</option></term>
<listitem>
<para>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index c1134eae5b..00aeb202ee 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -929,7 +929,8 @@ usage(void)
" --show-script=NAME show builtin script code, then exit\n"
" --verbose-errors print messages of all errors\n"
"\nCommon options:\n"
- " -d, --debug print debugging output\n"
+ " --debug print debugging output\n"
+ " -d, --dbname=DBNAME database name to connect to\n"
" -h, --host=HOSTNAME database server host or socket directory\n"
" -p, --port=PORT database server port number\n"
" -U, --username=USERNAME connect as specified database user\n"
@@ -6562,7 +6563,7 @@ main(int argc, char **argv)
{"builtin", required_argument, NULL, 'b'},
{"client", required_argument, NULL, 'c'},
{"connect", no_argument, NULL, 'C'},
- {"debug", no_argument, NULL, 'd'},
+ {"dbname", required_argument, NULL, 'd'},
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
@@ -6602,6 +6603,7 @@ main(int argc, char **argv)
{"failures-detailed", no_argument, NULL, 13},
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
+ {"debug", no_argument, NULL, 16},
{NULL, 0, NULL, 0}
};
@@ -6673,7 +6675,7 @@ main(int argc, char **argv)
if (!set_random_seed(getenv("PGBENCH_RANDOM_SEED")))
pg_fatal("error while setting random seed from PGBENCH_RANDOM_SEED environment variable");
- while ((c = getopt_long(argc, argv, "b:c:CdD:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "b:c:Cd:D:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
{
char *script;
@@ -6714,7 +6716,7 @@ main(int argc, char **argv)
is_connect = true;
break;
case 'd':
- pg_logging_increase_verbosity();
+ dbName = pg_strdup(optarg);
break;
case 'D':
{
@@ -6939,6 +6941,9 @@ main(int argc, char **argv)
benchmarking_option_set = true;
verbose_errors = true;
break;
+ case 16: /* debug */
+ pg_logging_increase_verbosity();
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -6989,16 +6994,19 @@ main(int argc, char **argv)
*/
throttle_delay *= nthreads;
- if (argc > optind)
- dbName = argv[optind++];
- else
+ if (dbName == NULL)
{
- if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
- dbName = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
- dbName = env;
+ if (argc > optind)
+ dbName = argv[optind++];
else
- dbName = get_user_name_or_exit(progname);
+ {
+ if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
+ dbName = env;
+ else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ dbName = env;
+ else
+ dbName = get_user_name_or_exit(progname);
+ }
}
if (optind < argc)
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 8c9a08e446..1db35a0aa2 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -1281,7 +1281,7 @@ my $err_pattern =
. "\\1";
$node->pgbench(
- "-n -c 2 -t 1 -d --verbose-errors --max-tries 2",
+ "-n -c 2 -t 1 --debug --verbose-errors --max-tries 2",
0,
[
qr{processed: 2/2\b},
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index 0ec54fbb03..3e293cb86a 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -67,9 +67,14 @@ my @options = (
# name, options, stderr checks
[
'bad option',
- '-h home -p 5432 -U calvin -d --bad-option',
+ '-h home -p 5432 -U calvin ---debug --bad-option',
[qr{--help.*more information}]
],
+ [
+ 'invalid database',
+ '-d nosuchdbname',
+ [qr{database "nosuchdbname" does not exist}]
+ ],
[
'no file',
'-f no-such-file',
On Thu, Feb 29, 2024 at 07:05:13PM -0500, Greg Sabino Mullane wrote:
Attached please find a patch to adjust the behavior of the pgbench program
and make it behave like the other programs that connect to a database
(namely, psql and pg_dump). Specifically, add support for using -d and
--dbname to specify the name of the database. This means that -d can no
longer be used to turn on debugging mode, and the long option --debug must
be used instead.This removes a long-standing footgun, in which people assume that the -d
option behaves the same as other programs. Indeed, because it takes no
arguments, and because the first non-option argument is the database name,
it still appears to work. However, people then wonder why pgbench is so
darn verbose all the time! :)This is a breaking change, but fixing it this way seems to have the least
total impact, as the number of people using the debug mode of pgbench is
likely quite small. Further, those already using the long option are
unaffected, and those using the short one simply need to replace '-d' with
'--debug', arguably making their scripts a little more self-documenting in
the process.
I think this is a generally reasonable proposal, except I don't know
whether this breakage is acceptable. AFAICT there are two fundamental
behavior changes folks would observe:
* "-d <database_name>" would cease to emit the debugging output, and while
enabling debug mode might've been unintentional in most cases, it might
actually have been intentional in others.
* "-d" with no argument or with a following option would begin failing, and
users would need to replace "-d" with "--debug".
Neither of these seems particularly severe to me, especially for a
benchmarking program, but I'd be curious to hear what others think.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On 3/1/24 23:41, Nathan Bossart wrote:
On Thu, Feb 29, 2024 at 07:05:13PM -0500, Greg Sabino Mullane wrote:
Attached please find a patch to adjust the behavior of the pgbench program
and make it behave like the other programs that connect to a database
(namely, psql and pg_dump). Specifically, add support for using -d and
--dbname to specify the name of the database. This means that -d can no
longer be used to turn on debugging mode, and the long option --debug must
be used instead.This removes a long-standing footgun, in which people assume that the -d
option behaves the same as other programs. Indeed, because it takes no
arguments, and because the first non-option argument is the database name,
it still appears to work. However, people then wonder why pgbench is so
darn verbose all the time! :)This is a breaking change, but fixing it this way seems to have the least
total impact, as the number of people using the debug mode of pgbench is
likely quite small. Further, those already using the long option are
unaffected, and those using the short one simply need to replace '-d' with
'--debug', arguably making their scripts a little more self-documenting in
the process.I think this is a generally reasonable proposal, except I don't know
whether this breakage is acceptable. AFAICT there are two fundamental
behavior changes folks would observe:* "-d <database_name>" would cease to emit the debugging output, and while
enabling debug mode might've been unintentional in most cases, it might
actually have been intentional in others.
I think this is the more severe of the two issues, because it's a silent
change. Everything will seem to work, but the user won't get the debug
info (if they actually wanted it).
* "-d" with no argument or with a following option would begin failing, and
users would need to replace "-d" with "--debug".
I think this is fine.
Neither of these seems particularly severe to me, especially for a
benchmarking program, but I'd be curious to hear what others think.
I agree the -d option may be confusing, but is it worth it? I don't
know, it depends on how often people actually get confused by it, and I
don't recall hitting this (nor hearing about others). To be honest I
didn't even realize pgbench even has a debug switch ...
But I'd like to mention this is far from our only tool using "-d" to
enable debug mode. A quick git-grep shows postgres, initdb,
pg_archivecleanup and pg_combinebackup do the same thing. So maybe it's
not that inconsistent overall.
(Note: I didn't check if the other cases may lead to the same confusion
where people enable debug accidentally. Maybe not.)
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Fri, Mar 1, 2024, at 8:07 PM, Tomas Vondra wrote:
On 3/1/24 23:41, Nathan Bossart wrote:
I think this is a generally reasonable proposal, except I don't know
whether this breakage is acceptable. AFAICT there are two fundamental
behavior changes folks would observe:* "-d <database_name>" would cease to emit the debugging output, and while
enabling debug mode might've been unintentional in most cases, it might
actually have been intentional in others.I think this is the more severe of the two issues, because it's a silent
change. Everything will seem to work, but the user won't get the debug
info (if they actually wanted it).
Indeed. Hopefully the user will notice soon when inspecting the standard error
output.
* "-d" with no argument or with a following option would begin failing, and
users would need to replace "-d" with "--debug".I think this is fine.
Yeah. It will force the user to fix it immediately.
Neither of these seems particularly severe to me, especially for a
benchmarking program, but I'd be curious to hear what others think.I agree the -d option may be confusing, but is it worth it? I don't
know, it depends on how often people actually get confused by it, and I
don't recall hitting this (nor hearing about others). To be honest I
didn't even realize pgbench even has a debug switch ...
I'm the one that has a habit to use -d to specify the database name. I
generally include -d for pgbench and then realized that I don't need the debug
information because it is not for database specification.
But I'd like to mention this is far from our only tool using "-d" to
enable debug mode. A quick git-grep shows postgres, initdb,
pg_archivecleanup and pg_combinebackup do the same thing. So maybe it's
not that inconsistent overall.
As Greg said none of these programs connects to the database.
I don't like to break backward compatibility but in this case I suspect that it
is ok. I don't recall the last time I saw a script that makes use of -d option.
How often do you need a pgbench debug information?
--
Euler Taveira
EDB https://www.enterprisedb.com/
On 2024-Mar-01, Euler Taveira wrote:
I don't like to break backward compatibility but in this case I suspect that it
is ok. I don't recall the last time I saw a script that makes use of -d option.
How often do you need a pgbench debug information?
I wondered what the difference actually is, so I checked. In -i mode,
the only difference is that if the tables don't exist before hand, we
receive the NOTICE that it doesn't. In normal mode, the -d switch emits
so much junk that I would believe if somebody told me that passing -d
distorted the benchmark results; and it's hard to believe that such
output is valuable for anything other than debugging pgbench itself.
All in all, I support the original patch.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"I love the Postgres community. It's all about doing things _properly_. :-)"
(David Garamond)
On Mon, Mar 11, 2024 at 04:59:36PM +0100, Alvaro Herrera wrote:
All in all, I support the original patch.
I'll commit this in a few days if there are no objections.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
On Tue, Mar 19, 2024 at 01:47:53PM -0500, Nathan Bossart wrote:
On Mon, Mar 11, 2024 at 04:59:36PM +0100, Alvaro Herrera wrote:
All in all, I support the original patch.
I'll commit this in a few days if there are no objections.
Actually, I just took a look at the patch and it appears to need a rebase
as well as additional documentation updates for the new -d/--dbname option.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Rebased version attached (v2), with another sentence in the sgml to explain
the optional use of -d
Cheers,
Greg
Attachments:
pgbench.dash.d.or.not.dash.d.v2.patchapplication/octet-stream; name=pgbench.dash.d.or.not.dash.d.v2.patchDownload
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 279bb0ad7d..023a01a46b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -165,8 +165,9 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
<term><replaceable class="parameter">dbname</replaceable></term>
<listitem>
<para>
- Specifies the name of the database to test in. If this is
- not specified, the environment variable
+ Specifies the name of the database to test in. The use
+ of <option>-d</option> before the name is optional.
+ If the name is not specified, the environment variable
<envar>PGDATABASE</envar> is used. If that is not set, the
user name specified for the connection is used.
</para>
@@ -463,7 +464,6 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</varlistentry>
<varlistentry id="pgbench-option-debug">
- <term><option>-d</option></term>
<term><option>--debug</option></term>
<listitem>
<para>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index af1f75257f..af776b31d8 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -933,7 +933,8 @@ usage(void)
" --show-script=NAME show builtin script code, then exit\n"
" --verbose-errors print messages of all errors\n"
"\nCommon options:\n"
- " -d, --debug print debugging output\n"
+ " --debug print debugging output\n"
+ " -d, --dbname=DBNAME database name to connect to\n"
" -h, --host=HOSTNAME database server host or socket directory\n"
" -p, --port=PORT database server port number\n"
" -U, --username=USERNAME connect as specified database user\n"
@@ -6620,7 +6621,7 @@ main(int argc, char **argv)
{"builtin", required_argument, NULL, 'b'},
{"client", required_argument, NULL, 'c'},
{"connect", no_argument, NULL, 'C'},
- {"debug", no_argument, NULL, 'd'},
+ {"dbname", required_argument, NULL, 'd'},
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
@@ -6661,6 +6662,7 @@ main(int argc, char **argv)
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
{"exit-on-abort", no_argument, NULL, 16},
+ {"debug", no_argument, NULL, 17},
{NULL, 0, NULL, 0}
};
@@ -6732,7 +6734,7 @@ main(int argc, char **argv)
if (!set_random_seed(getenv("PGBENCH_RANDOM_SEED")))
pg_fatal("error while setting random seed from PGBENCH_RANDOM_SEED environment variable");
- while ((c = getopt_long(argc, argv, "b:c:CdD:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "b:c:Cd:D:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
{
char *script;
@@ -6773,7 +6775,7 @@ main(int argc, char **argv)
is_connect = true;
break;
case 'd':
- pg_logging_increase_verbosity();
+ dbName = pg_strdup(optarg);
break;
case 'D':
{
@@ -6998,6 +7000,9 @@ main(int argc, char **argv)
benchmarking_option_set = true;
exit_on_abort = true;
break;
+ case 17: /* debug */
+ pg_logging_increase_verbosity();
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -7048,16 +7053,19 @@ main(int argc, char **argv)
*/
throttle_delay *= nthreads;
- if (argc > optind)
- dbName = argv[optind++];
- else
+ if (dbName == NULL)
{
- if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
- dbName = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
- dbName = env;
+ if (argc > optind)
+ dbName = argv[optind++];
else
- dbName = get_user_name_or_exit(progname);
+ {
+ if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
+ dbName = env;
+ else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ dbName = env;
+ else
+ dbName = get_user_name_or_exit(progname);
+ }
}
if (optind < argc)
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 5d2341a203..d0a86a280c 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -1352,7 +1352,7 @@ my $err_pattern =
. "\\1";
$node->pgbench(
- "-n -c 2 -t 1 -d --verbose-errors --max-tries 2",
+ "-n -c 2 -t 1 --debug --verbose-errors --max-tries 2",
0,
[
qr{processed: 2/2\b},
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index e0e4d92b06..108ea7af4b 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -66,9 +66,14 @@ my @options = (
# name, options, stderr checks
[
'bad option',
- '-h home -p 5432 -U calvin -d --bad-option',
+ '-h home -p 5432 -U calvin ---debug --bad-option',
[qr{--help.*more information}]
],
+ [
+ 'invalid database',
+ '-d nosuchdbname',
+ [qr{database "nosuchdbname" does not exist}]
+ ],
[
'no file',
'-f no-such-file',
On Tue, Mar 19, 2024 at 09:15:22PM -0400, Greg Sabino Mullane wrote:
Rebased version attached (v2), with another sentence in the sgml to explain
the optional use of -d
cfbot seems quite unhappy with this:
https://cirrus-ci.com/build/6429518263484416
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
My mistake. Attached please find version 3, which should hopefully make
cfbot happy again.
Attachments:
pgbench.dash.d.or.not.dash.d.v3.patchapplication/octet-stream; name=pgbench.dash.d.or.not.dash.d.v3.patchDownload
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 279bb0ad7d..023a01a46b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -165,8 +165,9 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
<term><replaceable class="parameter">dbname</replaceable></term>
<listitem>
<para>
- Specifies the name of the database to test in. If this is
- not specified, the environment variable
+ Specifies the name of the database to test in. The use
+ of <option>-d</option> before the name is optional.
+ If the name is not specified, the environment variable
<envar>PGDATABASE</envar> is used. If that is not set, the
user name specified for the connection is used.
</para>
@@ -463,7 +464,6 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</varlistentry>
<varlistentry id="pgbench-option-debug">
- <term><option>-d</option></term>
<term><option>--debug</option></term>
<listitem>
<para>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index af1f75257f..af776b31d8 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -933,7 +933,8 @@ usage(void)
" --show-script=NAME show builtin script code, then exit\n"
" --verbose-errors print messages of all errors\n"
"\nCommon options:\n"
- " -d, --debug print debugging output\n"
+ " --debug print debugging output\n"
+ " -d, --dbname=DBNAME database name to connect to\n"
" -h, --host=HOSTNAME database server host or socket directory\n"
" -p, --port=PORT database server port number\n"
" -U, --username=USERNAME connect as specified database user\n"
@@ -6620,7 +6621,7 @@ main(int argc, char **argv)
{"builtin", required_argument, NULL, 'b'},
{"client", required_argument, NULL, 'c'},
{"connect", no_argument, NULL, 'C'},
- {"debug", no_argument, NULL, 'd'},
+ {"dbname", required_argument, NULL, 'd'},
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
@@ -6661,6 +6662,7 @@ main(int argc, char **argv)
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
{"exit-on-abort", no_argument, NULL, 16},
+ {"debug", no_argument, NULL, 17},
{NULL, 0, NULL, 0}
};
@@ -6732,7 +6734,7 @@ main(int argc, char **argv)
if (!set_random_seed(getenv("PGBENCH_RANDOM_SEED")))
pg_fatal("error while setting random seed from PGBENCH_RANDOM_SEED environment variable");
- while ((c = getopt_long(argc, argv, "b:c:CdD:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "b:c:Cd:D:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
{
char *script;
@@ -6773,7 +6775,7 @@ main(int argc, char **argv)
is_connect = true;
break;
case 'd':
- pg_logging_increase_verbosity();
+ dbName = pg_strdup(optarg);
break;
case 'D':
{
@@ -6998,6 +7000,9 @@ main(int argc, char **argv)
benchmarking_option_set = true;
exit_on_abort = true;
break;
+ case 17: /* debug */
+ pg_logging_increase_verbosity();
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -7048,16 +7053,19 @@ main(int argc, char **argv)
*/
throttle_delay *= nthreads;
- if (argc > optind)
- dbName = argv[optind++];
- else
+ if (dbName == NULL)
{
- if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
- dbName = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
- dbName = env;
+ if (argc > optind)
+ dbName = argv[optind++];
else
- dbName = get_user_name_or_exit(progname);
+ {
+ if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
+ dbName = env;
+ else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ dbName = env;
+ else
+ dbName = get_user_name_or_exit(progname);
+ }
}
if (optind < argc)
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 5d2341a203..b33ca058d1 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -79,6 +79,16 @@ $node->pgbench(
],
'no such database');
+$node->pgbench(
+ '-d no-such-database',
+ 1,
+ [qr{^$}],
+ [
+ qr{connection to server .* failed},
+ qr{FATAL: database "no-such-database" does not exist}
+ ],
+ 'no such database (-d)');
+
$node->pgbench(
'-S -t 1', 1, [],
[qr{Perhaps you need to do initialization}],
@@ -1352,7 +1362,7 @@ my $err_pattern =
. "\\1";
$node->pgbench(
- "-n -c 2 -t 1 -d --verbose-errors --max-tries 2",
+ "-n -c 2 -t 1 --debug --verbose-errors --max-tries 2",
0,
[
qr{processed: 2/2\b},
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index e0e4d92b06..a04b3531f4 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -66,7 +66,7 @@ my @options = (
# name, options, stderr checks
[
'bad option',
- '-h home -p 5432 -U calvin -d --bad-option',
+ '-h home -p 5432 -U calvin ---debug --bad-option',
[qr{--help.*more information}]
],
[
The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: tested, passed
Did a quick review of this one; CFbot is now happy, local regression tests all pass.
I think the idea here is sane; it's particularly confusing for some tools included in the main distribution to have different semantics, and this seems low on the breakage risk here, so worth the tradeoffs IMO.
The new status of this patch is: Ready for Committer
On Wed, Mar 20, 2024 at 11:57:25AM -0400, Greg Sabino Mullane wrote:
My mistake. Attached please find version 3, which should hopefully make
cfbot happy again.
Here is what I have staged for commit. I plan to commit this within the
next few days.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Attachments:
v4-0001-pgbench-Disambiguate-d-option.patchtext/x-diff; charset=us-asciiDownload
From 25523d7513615128ef234c03c87612c3b5eb98e5 Mon Sep 17 00:00:00 2001
From: Nathan Bossart <nathan@postgresql.org>
Date: Thu, 21 Mar 2024 16:06:03 -0500
Subject: [PATCH v4 1/1] pgbench: Disambiguate -d option.
Many other utilities use -d to specify the database to use, but
pgbench uses it to enable debugging mode. This is causing some
users to accidentally enable debugging mode. This commit moves the
debugging mode to --debug, converts -d to accept the database name,
and introduces --dbname. This is a backward-incompatible change,
but that has been judged to be worth the trade-off.
Author: Greg Sabino Mullane
Reviewed-by: Tomas Vondra, Euler Taveira, Alvaro Herrera, David Christensen
Discussion: https://postgr.es/m/CAKAnmmLjAzwVtb%3DVEaeuCtnmOLpzkJ1uJ_XiQ362YdD9B72HSg%40mail.gmail.com
---
doc/src/sgml/ref/pgbench.sgml | 4 +--
src/bin/pgbench/pgbench.c | 32 ++++++++++++--------
src/bin/pgbench/t/001_pgbench_with_server.pl | 2 +-
src/bin/pgbench/t/002_pgbench_no_server.pl | 2 +-
4 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 279bb0ad7d..c3d0ec240b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -162,7 +162,8 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
<variablelist>
<varlistentry id="pgbench-option-dbname">
- <term><replaceable class="parameter">dbname</replaceable></term>
+ <term><option><optional>-d</optional> <replaceable class="parameter">dbname</replaceable></option></term>
+ <term><option><optional>--dbname=</optional><replaceable class="parameter">dbname</replaceable></option></term>
<listitem>
<para>
Specifies the name of the database to test in. If this is
@@ -463,7 +464,6 @@ pgbench <optional> <replaceable>options</replaceable> </optional> <replaceable>d
</varlistentry>
<varlistentry id="pgbench-option-debug">
- <term><option>-d</option></term>
<term><option>--debug</option></term>
<listitem>
<para>
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index af1f75257f..af776b31d8 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -933,7 +933,8 @@ usage(void)
" --show-script=NAME show builtin script code, then exit\n"
" --verbose-errors print messages of all errors\n"
"\nCommon options:\n"
- " -d, --debug print debugging output\n"
+ " --debug print debugging output\n"
+ " -d, --dbname=DBNAME database name to connect to\n"
" -h, --host=HOSTNAME database server host or socket directory\n"
" -p, --port=PORT database server port number\n"
" -U, --username=USERNAME connect as specified database user\n"
@@ -6620,7 +6621,7 @@ main(int argc, char **argv)
{"builtin", required_argument, NULL, 'b'},
{"client", required_argument, NULL, 'c'},
{"connect", no_argument, NULL, 'C'},
- {"debug", no_argument, NULL, 'd'},
+ {"dbname", required_argument, NULL, 'd'},
{"define", required_argument, NULL, 'D'},
{"file", required_argument, NULL, 'f'},
{"fillfactor", required_argument, NULL, 'F'},
@@ -6661,6 +6662,7 @@ main(int argc, char **argv)
{"max-tries", required_argument, NULL, 14},
{"verbose-errors", no_argument, NULL, 15},
{"exit-on-abort", no_argument, NULL, 16},
+ {"debug", no_argument, NULL, 17},
{NULL, 0, NULL, 0}
};
@@ -6732,7 +6734,7 @@ main(int argc, char **argv)
if (!set_random_seed(getenv("PGBENCH_RANDOM_SEED")))
pg_fatal("error while setting random seed from PGBENCH_RANDOM_SEED environment variable");
- while ((c = getopt_long(argc, argv, "b:c:CdD:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
+ while ((c = getopt_long(argc, argv, "b:c:Cd:D:f:F:h:iI:j:lL:M:nNp:P:qrR:s:St:T:U:v", long_options, &optindex)) != -1)
{
char *script;
@@ -6773,7 +6775,7 @@ main(int argc, char **argv)
is_connect = true;
break;
case 'd':
- pg_logging_increase_verbosity();
+ dbName = pg_strdup(optarg);
break;
case 'D':
{
@@ -6998,6 +7000,9 @@ main(int argc, char **argv)
benchmarking_option_set = true;
exit_on_abort = true;
break;
+ case 17: /* debug */
+ pg_logging_increase_verbosity();
+ break;
default:
/* getopt_long already emitted a complaint */
pg_log_error_hint("Try \"%s --help\" for more information.", progname);
@@ -7048,16 +7053,19 @@ main(int argc, char **argv)
*/
throttle_delay *= nthreads;
- if (argc > optind)
- dbName = argv[optind++];
- else
+ if (dbName == NULL)
{
- if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
- dbName = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
- dbName = env;
+ if (argc > optind)
+ dbName = argv[optind++];
else
- dbName = get_user_name_or_exit(progname);
+ {
+ if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
+ dbName = env;
+ else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ dbName = env;
+ else
+ dbName = get_user_name_or_exit(progname);
+ }
}
if (optind < argc)
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index 5d2341a203..d0a86a280c 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -1352,7 +1352,7 @@ my $err_pattern =
. "\\1";
$node->pgbench(
- "-n -c 2 -t 1 -d --verbose-errors --max-tries 2",
+ "-n -c 2 -t 1 --debug --verbose-errors --max-tries 2",
0,
[
qr{processed: 2/2\b},
diff --git a/src/bin/pgbench/t/002_pgbench_no_server.pl b/src/bin/pgbench/t/002_pgbench_no_server.pl
index e0e4d92b06..a04b3531f4 100644
--- a/src/bin/pgbench/t/002_pgbench_no_server.pl
+++ b/src/bin/pgbench/t/002_pgbench_no_server.pl
@@ -66,7 +66,7 @@ my @options = (
# name, options, stderr checks
[
'bad option',
- '-h home -p 5432 -U calvin -d --bad-option',
+ '-h home -p 5432 -U calvin ---debug --bad-option',
[qr{--help.*more information}]
],
[
--
2.25.1
Committed.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com