[PATCH] pgbench: Bug fix for the -d option
Hi.
I found a bug in pgbench's -d option and created a patch.
The bug is the following:
pgbench's option -d can display debug log about connection, which is
like "pghost: foo pgport: 5432 nclients: 1 nxacts: 10 dbName: bar".
This configuration is supplied by other options or environment variables
like PGUSER or PGPORT.
When there is no PGDATABASE, pgbench will use PGUSER as the dbName.
However, when there is PGPORT environment variable, this debug logger
doesn't refer PGUSER even if there is PGUSER.
In other words, even if you are setting both PGPORT and PGUSER,
pgbench's option -d will display like this: "pghost: foo pgport: 5432
nclients: 1 nxact: 10 dbName: ".
I think this is a bug that dbName is displayed as if it's not specified.
Note that this bug is only related to this debug logger. The main unit
of pgbench can establish a connection by complementing dbName with
PGUSER despite this bug.
So I made a patch (only one line changed).
As shown in this patch file, I just changed the else if statement to an
if statement.
I'm suggesting this bug fix because I think it's a bug, but if there's
any other intent to this else if statement, could you let me know?
Regards
---
Kota Miyake
Attachments:
pgbench_debuglogger.patchtext/x-diff; name=pgbench_debuglogger.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 627a244fb7..6a681b131b 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -5491,7 +5491,7 @@ main(int argc, char **argv)
pghost = env;
if ((env = getenv("PGPORT")) != NULL && *env != '\0')
pgport = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ if ((env = getenv("PGUSER")) != NULL && *env != '\0')
login = env;
state = (CState *) pg_malloc0(sizeof(CState));
On Fri, Feb 26, 2021 at 01:18:20PM +0900, miyake_kouta wrote:
I'm suggesting this bug fix because I think it's a bug, but if there's any
other intent to this else if statement, could you let me know?
Yes, the existing code could mess up with the selection logic if
PGPORT and PGUSER are both specified in an environment, masking the
value of PGUSER, so let's fix that. This is as old as 412893b.
--
Michael
On Fri, Feb 26, 2021 at 05:16:17PM +0900, Michael Paquier wrote:
Yes, the existing code could mess up with the selection logic if
PGPORT and PGUSER are both specified in an environment, masking the
value of PGUSER, so let's fix that. This is as old as 412893b.
By the way, I can help but wonder why pgbench has such a different
handling for the user name, fetching first PGUSER and then looking at
the options while most of the other binaries use get_user_name(). It
seems to me that we could simplify the handling around "login" without
really impacting the usefulness of the tool, no?
--
Michael
2021-02-26 20:30, Michael Paquier wrote:
By the way, I can help but wonder why pgbench has such a different
handling for the user name, fetching first PGUSER and then looking at
the options while most of the other binaries use get_user_name(). It
seems to me that we could simplify the handling around "login" without
really impacting the usefulness of the tool, no?
Hi.
Thank you for your comment.
I modified the patch based on other binaries.
In this new patch, if there is a $PGUSER, then it's set to login.
If not, get_user_name_or_exit is excuted.
Plese let me know what you think about this change.
--
Kota Miyake
Attachments:
v02_pgbench_debuglogger.patchtext/x-diff; name=v02_pgbench_debuglogger.patchDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31a4df45f5..b20c7b5b27 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
#endif
#include "postgres_fe.h"
+#include "common/username.h"
#include <ctype.h>
#include <float.h>
@@ -5487,8 +5488,10 @@ main(int argc, char **argv)
pghost = env;
if ((env = getenv("PGPORT")) != NULL && *env != '\0')
pgport = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ if ((env = getenv("PGUSER")) != NULL && *env != '\0')
login = env;
+ else
+ login = get_user_name_or_exit(progname);
state = (CState *) pg_malloc0(sizeof(CState));
On Tue, Mar 02, 2021 at 11:52:33AM +0900, miyake_kouta wrote:
I modified the patch based on other binaries.
In this new patch, if there is a $PGUSER, then it's set to login.
If not, get_user_name_or_exit is excuted.
Plese let me know what you think about this change.
Your patch makes the database selection slightly better, but I think
that we can do better and simpler than that. So please see the
attached.
One thing on HEAD that looks like a bug to me is that if one uses a
pgbench command without specifying user, port and/or name in the
command for an environment without PGDATABASE, PGPORT and PGHOST set,
then the debug log just before doConnect() prints empty strings for
all that, which is basically useless so one has no idea where the
connection happens. Like any other src/bin/ facilities, let's instead
remove all the getenv() calls at the beginning of pgbench.c and let's
libpq handle those environment variables if the parameters are NULL
(aka in the case of no values given directly in the options). This
requires to move the debug log after doConnect(), which is no big deal
anyway as a failure results in an exit(1) immediately with a log
telling where the connection failed.
What do you think?
--
Michael
Attachments:
v03_pgbench_debuglogger.patchtext/x-diff; charset=us-asciiDownload
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31a4df45f5..ab56272f2e 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
#endif
#include "postgres_fe.h"
+#include "common/username.h"
#include <ctype.h>
#include <float.h>
@@ -240,10 +241,10 @@ bool is_connect; /* establish connection for each transaction */
bool report_per_command; /* report per-command latencies */
int main_pid; /* main process id used in log filename */
-char *pghost = "";
-char *pgport = "";
-char *login = NULL;
-char *dbName;
+const char *pghost = NULL;
+const char *pgport = NULL;
+const char *username = NULL;
+const char *dbName = NULL;
char *logfile_prefix = NULL;
const char *progname;
@@ -1191,7 +1192,7 @@ doConnect(void)
keywords[1] = "port";
values[1] = pgport;
keywords[2] = "user";
- values[2] = login;
+ values[2] = username;
keywords[3] = "password";
values[3] = password;
keywords[4] = "dbname";
@@ -5483,13 +5484,6 @@ main(int argc, char **argv)
}
}
- if ((env = getenv("PGHOST")) != NULL && *env != '\0')
- pghost = env;
- if ((env = getenv("PGPORT")) != NULL && *env != '\0')
- pgport = env;
- else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
- login = env;
-
state = (CState *) pg_malloc0(sizeof(CState));
/* set random seed early, because it may be used while parsing scripts. */
@@ -5610,7 +5604,7 @@ main(int argc, char **argv)
}
break;
case 'U':
- login = pg_strdup(optarg);
+ username = pg_strdup(optarg);
break;
case 'l':
benchmarking_option_set = true;
@@ -5860,10 +5854,10 @@ main(int argc, char **argv)
{
if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
dbName = env;
- else if (login != NULL && *login != '\0')
- dbName = login;
+ else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ dbName = env;
else
- dbName = "";
+ dbName = get_user_name_or_exit(progname);
}
if (optind < argc)
@@ -6026,16 +6020,16 @@ main(int argc, char **argv)
initRandomState(&state[i].cs_func_rs);
}
- pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
- pghost, pgport, nclients,
- duration <= 0 ? "nxacts" : "duration",
- duration <= 0 ? nxacts : duration, dbName);
-
/* opening connection... */
con = doConnect();
if (con == NULL)
exit(1);
+ pg_log_debug("pghost: %s pgport: %s nclients: %d %s: %d dbName: %s",
+ PQhost(con), PQport(con), nclients,
+ duration <= 0 ? "nxacts" : "duration",
+ duration <= 0 ? nxacts : duration, PQdb(con));
+
if (internal_script_used)
GetTableInfo(con, scale_given);
2021-03-04 21:11, Michael Paquier wrote:
Like any other src/bin/ facilities, let's instead
remove all the getenv() calls at the beginning of pgbench.c and let's
libpq handle those environment variables if the parameters are NULL
(aka in the case of no values given directly in the options). This
requires to move the debug log after doConnect(), which is no big deal
anyway as a failure results in an exit(1) immediately with a log
telling where the connection failed.
Thank you for improving my patch.
I agree that we should remove getenv() from pgbench.c and let libpq
complement parameters with environment variables.
As you said, it's not a big problem that the debug log output comes
after doConnect(), I think.
Your patch is simpler and more ideal.
Regards.
--
Kota Miyake
On 2021/03/05 11:26, miyake_kouta wrote:
2021-03-04 21:11, Michael Paquier wrote:
Like any other src/bin/ facilities, let's instead
remove all the getenv() calls at the beginning of pgbench.c and let's
libpq handle those environment variables if the parameters are NULL
(aka in the case of no values given directly in the options). This
requires to move the debug log after doConnect(), which is no big deal
anyway as a failure results in an exit(1) immediately with a log
telling where the connection failed.
if ((env = getenv("PGDATABASE")) != NULL && *env != '\0')
dbName = env;
- else if (login != NULL && *login != '\0')
- dbName = login;
+ else if ((env = getenv("PGUSER")) != NULL && *env != '\0')
+ dbName = env;
else
- dbName = "";
+ dbName = get_user_name_or_exit(progname);If dbName is set by libpq, the above also is not necessary?
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Mar 05, 2021 at 01:30:11PM +0900, Fujii Masao wrote:
if ((env = getenv("PGDATABASE")) != NULL && *env != '\0') dbName = env; - else if (login != NULL && *login != '\0') - dbName = login; + else if ((env = getenv("PGUSER")) != NULL && *env != '\0') + dbName = env; else - dbName = ""; + dbName = get_user_name_or_exit(progname);If dbName is set by libpq, the above also is not necessary?
If you remove this part, pgbench loses some log information if
PQconnectdbParams() returns NULL, like on OOM if the database name is
NULL. Perhaps that's not worth caring about here for a single log
failure, but that's the reason is why I left this part around.
Now, simplifying the code is one goal of this patch, so I would not
mind shaving a bit more of it :)
--
Michael
On 2021/03/05 16:33, Michael Paquier wrote:
On Fri, Mar 05, 2021 at 01:30:11PM +0900, Fujii Masao wrote:
if ((env = getenv("PGDATABASE")) != NULL && *env != '\0') dbName = env; - else if (login != NULL && *login != '\0') - dbName = login; + else if ((env = getenv("PGUSER")) != NULL && *env != '\0') + dbName = env; else - dbName = ""; + dbName = get_user_name_or_exit(progname);If dbName is set by libpq, the above also is not necessary?
If you remove this part, pgbench loses some log information if
PQconnectdbParams() returns NULL, like on OOM if the database name is
NULL. Perhaps that's not worth caring about here for a single log
failure, but that's the reason is why I left this part around.
Understood. Thanks!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Mar 05, 2021 at 06:35:47PM +0900, Fujii Masao wrote:
Understood. Thanks!
Okay, so I have gone through this stuff today, and applied the
simplification. Thanks.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Fri, Mar 05, 2021 at 06:35:47PM +0900, Fujii Masao wrote:
Understood. Thanks!
Okay, so I have gone through this stuff today, and applied the
simplification. Thanks.
This item is still open according to the commitfest app ---
should that entry be closed?
regards, tom lane