Hi all,
In six places of the code tree (+ one in psql which is a bit
different), we have the following pattern for frontend tools to
connect to a backend with a password prompt, roughly like that:
do
{
[...]
conn = PQconnectdbParams(keywords, values, true);
[...]
if (PQstatus(conn) == CONNECTION_BAD &&
PQconnectionNeedsPassword(conn) &&
!have_password)
{
PQfinish(conn);
simple_prompt("Password: ", password, sizeof(password), false);
have_password = true;
new_pass = true;
}
} while (new_pass);
Attached is a tentative of patch to consolidate this logic across the
tree. The patch is far from being in a committable state, and there
are a couple of gotchas:
- pg_dumpall merges connection string parameters, so it is much harder
to plugin that the others, and I left it out on purpose.
- Some code paths spawn a password prompt before the first connection
attempt. For now I have left this first attempt out of the refactored
logic, but I think that it is possible to consolidate that a bit more
by enforcing a password prompt before doing the first connection
attempt (somewhat related to the XXX portion in the patch). At the
end it would be nice to not have to have a 100-byte-long field for the
password buffer we have here and there. Unfortunately this comes with
its limitations in pg_dump as savedPassword needs to be moved around
and may be reused in the context.
- I don't like the routine name connect_with_password_prompt() I
introduced. Suggestions of better names are welcome :)
- This also brings the point that some of our tools are not able to
handle tri-values for passwords, so we may want to consolidate that as
well.
Among the positive points, this brings a lot of consolidation in terms
of error handling, and this shaves a bit of code:
13 files changed, 190 insertions(+), 280 deletions(-)
This moves the logic into src/fe_utils, which is natural as that's
aimed only for frontends and because this also links to libpq.
Please note that this links a bit with the refactoring of vacuumlo and
oid2name logging I proposed a couple of days back (applying one patch
or the other results in conflicts) because we need to have frontends
initialized for logging in order to be able to log errors in the
refactored routine:
https://www.postgresql.org/message-id/20190820012819.ga8...@paquier.xyz
This one should be merged first IMO, but that's a story for the other
thread.
This compiles, and passes all regression tests so it is possible to
play with it easily, still it needs much more testing and love.
Any thoughts? I am adding that to the next commit fest.
Thanks,
--
Michael
diff --git a/contrib/oid2name/Makefile b/contrib/oid2name/Makefile
index 361a80a7a1..c6ba092f47 100644
--- a/contrib/oid2name/Makefile
+++ b/contrib/oid2name/Makefile
@@ -9,7 +9,7 @@ OBJS = oid2name.o $(WIN32RES)
TAP_TESTS = 1
PG_CPPFLAGS = -I$(libpq_srcdir)
-PG_LIBS_INTERNAL = $(libpq_pgport)
+PG_LIBS_INTERNAL = $(libpq_pgport) -L$(top_builddir)/src/fe_utils -lpgfeutils
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/oid2name/oid2name.c b/contrib/oid2name/oid2name.c
index d3a4a50005..c38679e869 100644
--- a/contrib/oid2name/oid2name.c
+++ b/contrib/oid2name/oid2name.c
@@ -11,6 +11,7 @@
#include "catalog/pg_class_d.h"
+#include "common/logging.h"
#include "fe_utils/connect.h"
#include "libpq-fe.h"
#include "pg_getopt.h"
@@ -46,6 +47,8 @@ struct options
const char *progname;
};
+const char *progname;
+
/* function prototypes */
static void help(const char *progname);
void get_opts(int, char **, struct options *);
@@ -82,9 +85,9 @@ get_opts(int argc, char **argv, struct options *my_opts)
};
int c;
- const char *progname;
int optindex;
+ pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
/* set the defaults */
@@ -284,65 +287,20 @@ PGconn *
sql_conn(struct options *my_opts)
{
PGconn *conn;
- bool have_password = false;
- char password[100];
- bool new_pass;
PGresult *res;
- /*
- * Start the connection. Loop until we have a password if requested by
- * backend.
- */
- do
+ /* grab a connection, with password prompting */
+ conn = connect_with_password_prompt(my_opts->hostname,
+ my_opts->port,
+ my_opts->username,
+ my_opts->dbname,
+ my_opts->progname,
+ NULL,
+ true);
+ if (conn == NULL)
{
-#define PARAMS_ARRAY_SIZE 7
-
- const char *keywords[PARAMS_ARRAY_SIZE];
- const char *values[PARAMS_ARRAY_SIZE];
-
- keywords[0] = "host";
- values[0] = my_opts->hostname;
- keywords[1] = "port";
- values[1] = my_opts->port;
- keywords[2] = "user";
- values[2] = my_opts->username;
- keywords[3] = "password";
- values[3] = have_password ? password : NULL;
- keywords[4] = "dbname";
- values[4] = my_opts->dbname;
- keywords[5] = "fallback_application_name";
- values[5] = my_opts->progname;
- keywords[6] = NULL;
- values[6] = NULL;
-
- new_pass = false;
- conn = PQconnectdbParams(keywords, values, true);
-
- if (!conn)
- {
- fprintf(stderr, "%s: could not connect to database %s\n",
- "oid2name", my_opts->dbname);
- exit(1);
- }
-
- if (PQstatus(conn) == CONNECTION_BAD &&
- PQconnectionNeedsPassword(conn) &&
- !have_password)
- {
- PQfinish(conn);
- simple_prompt("Password: ", password, sizeof(password), false);
- have_password = true;
- new_pass = true;
- }
- } while (new_pass);
-
- /* check to see that the backend connection was successfully made */
- if (PQstatus(conn) == CONNECTION_BAD)
- {
- fprintf(stderr, "%s: could not connect to database %s: %s",
- "oid2name", my_opts->dbname, PQerrorMessage(conn));
- PQfinish(conn);
- exit(1);
+ /* an error has been generated */
+ exit(-1);
}
res = PQexec(conn, ALWAYS_SECURE_SEARCH_PATH_SQL);
diff --git a/contrib/vacuumlo/Makefile b/contrib/vacuumlo/Makefile
index 3efcb46735..2515f61824 100644
--- a/contrib/vacuumlo/Makefile
+++ b/contrib/vacuumlo/Makefile
@@ -9,7 +9,7 @@ OBJS = vacuumlo.o $(WIN32RES)
TAP_TESTS = 1
PG_CPPFLAGS = -I$(libpq_srcdir)
-PG_LIBS_INTERNAL = $(libpq_pgport)
+PG_LIBS_INTERNAL = $(libpq_pgport) -L$(top_builddir)/src/fe_utils -lpgfeutils
ifdef USE_PGXS
PG_CONFIG = pg_config
diff --git a/contrib/vacuumlo/vacuumlo.c b/contrib/vacuumlo/vacuumlo.c
index 73c06a043e..a236d4ae35 100644
--- a/contrib/vacuumlo/vacuumlo.c
+++ b/contrib/vacuumlo/vacuumlo.c
@@ -23,6 +23,7 @@
#include "catalog/pg_class_d.h"
+#include "common/logging.h"
#include "fe_utils/connect.h"
#include "libpq-fe.h"
#include "pg_getopt.h"
@@ -49,6 +50,8 @@ struct _param
long transaction_limit;
};
+const char *progname;
+
static int vacuumlo(const char *database, const struct _param *param);
static void usage(const char *progname);
@@ -67,7 +70,6 @@ vacuumlo(const char *database, const struct _param *param)
long matched;
long deleted;
int i;
- bool new_pass;
bool success = true;
static bool have_password = false;
static char password[100];
@@ -79,59 +81,18 @@ vacuumlo(const char *database, const struct _param *param)
have_password = true;
}
- /*
- * Start the connection. Loop until we have a password if requested by
- * backend.
- */
- do
+ /* grab a connection, depending on password prompting */
+ conn = connect_with_password_prompt(param->pg_host,
+ param->pg_port,
+ param->pg_user,
+ database,
+ progname,
+ password,
+ param->pg_prompt != TRI_NO);
+
+ if (conn == NULL)
{
-#define PARAMS_ARRAY_SIZE 7
-
- const char *keywords[PARAMS_ARRAY_SIZE];
- const char *values[PARAMS_ARRAY_SIZE];
-
- keywords[0] = "host";
- values[0] = param->pg_host;
- keywords[1] = "port";
- values[1] = param->pg_port;
- keywords[2] = "user";
- values[2] = param->pg_user;
- keywords[3] = "password";
- values[3] = have_password ? password : NULL;
- keywords[4] = "dbname";
- values[4] = database;
- keywords[5] = "fallback_application_name";
- values[5] = param->progname;
- keywords[6] = NULL;
- values[6] = NULL;
-
- new_pass = false;
- conn = PQconnectdbParams(keywords, values, true);
- if (!conn)
- {
- fprintf(stderr, "Connection to database \"%s\" failed\n",
- database);
- return -1;
- }
-
- if (PQstatus(conn) == CONNECTION_BAD &&
- PQconnectionNeedsPassword(conn) &&
- !have_password &&
- param->pg_prompt != TRI_NO)
- {
- PQfinish(conn);
- simple_prompt("Password: ", password, sizeof(password), false);
- have_password = true;
- new_pass = true;
- }
- } while (new_pass);
-
- /* check to see that the backend connection was successfully made */
- if (PQstatus(conn) == CONNECTION_BAD)
- {
- fprintf(stderr, "Connection to database \"%s\" failed:\n%s",
- database, PQerrorMessage(conn));
- PQfinish(conn);
+ /* an error has been generated */
return -1;
}
@@ -468,9 +429,9 @@ main(int argc, char **argv)
struct _param param;
int c;
int port;
- const char *progname;
int optindex;
+ pg_logging_init(argv[0]);
progname = get_progname(argv[0]);
/* Set default parameter values */
diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
index ee822c5249..20a849a8d2 100644
--- a/src/bin/pg_dump/pg_backup_db.c
+++ b/src/bin/pg_dump/pg_backup_db.c
@@ -245,7 +245,6 @@ ConnectDatabase(Archive *AHX,
ArchiveHandle *AH = (ArchiveHandle *) AHX;
char *password;
char passbuf[100];
- bool new_pass;
if (AH->connection)
fatal("already connected to a database");
@@ -259,53 +258,18 @@ ConnectDatabase(Archive *AHX,
}
AH->promptPassword = prompt_password;
- /*
- * Start the connection. Loop until we have a password if requested by
- * backend.
- */
- do
+ AH->connection = connect_with_password_prompt(pghost,
+ pgport,
+ username,
+ dbname,
+ progname,
+ password,
+ prompt_password != TRI_NO);
+ if (AH->connection == NULL)
{
- const char *keywords[7];
- const char *values[7];
-
- keywords[0] = "host";
- values[0] = pghost;
- keywords[1] = "port";
- values[1] = pgport;
- keywords[2] = "user";
- values[2] = username;
- keywords[3] = "password";
- values[3] = password;
- keywords[4] = "dbname";
- values[4] = dbname;
- keywords[5] = "fallback_application_name";
- values[5] = progname;
- keywords[6] = NULL;
- values[6] = NULL;
-
- new_pass = false;
- AH->connection = PQconnectdbParams(keywords, values, true);
-
- if (!AH->connection)
- fatal("could not connect to database");
-
- if (PQstatus(AH->connection) == CONNECTION_BAD &&
- PQconnectionNeedsPassword(AH->connection) &&
- password == NULL &&
- prompt_password != TRI_NO)
- {
- PQfinish(AH->connection);
- simple_prompt("Password: ", passbuf, sizeof(passbuf), false);
- password = passbuf;
- new_pass = true;
- }
- } while (new_pass);
-
- /* check to see that the backend connection was successfully made */
- if (PQstatus(AH->connection) == CONNECTION_BAD)
- fatal("connection to database \"%s\" failed: %s",
- PQdb(AH->connection) ? PQdb(AH->connection) : "",
- PQerrorMessage(AH->connection));
+ /* an error has been generated */
+ exit_nicely(1);
+ }
/* Start strict; later phases may override this. */
PQclear(ExecuteSqlQueryForSingleRow((Archive *) AH,
diff --git a/src/bin/pg_dump/t/002_pg_dump.pl b/src/bin/pg_dump/t/002_pg_dump.pl
index f064ea1063..9161ba6ebb 100644
--- a/src/bin/pg_dump/t/002_pg_dump.pl
+++ b/src/bin/pg_dump/t/002_pg_dump.pl
@@ -3411,7 +3411,7 @@ foreach my $db (sort keys %create_sql)
command_fails_like(
[ 'pg_dump', '-p', "$port", 'qqq' ],
- qr/\Qpg_dump: error: connection to database "qqq" failed: FATAL: database "qqq" does not exist\E/,
+ qr/\Qpg_dump: error: could not connect to database "qqq": FATAL: database "qqq" does not exist\E/,
'connecting to a non-existent database');
#########################################
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 570cf3306a..98885e0403 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -35,6 +35,7 @@
#include "common/int.h"
#include "common/logging.h"
#include "fe_utils/conditional.h"
+#include "fe_utils/connect.h"
#include "getopt_long.h"
#include "libpq-fe.h"
#include "portability/instr_time.h"
@@ -1151,67 +1152,15 @@ static PGconn *
doConnect(void)
{
PGconn *conn;
- bool new_pass;
- static bool have_password = false;
- static char password[100];
-
- /*
- * Start the connection. Loop until we have a password if requested by
- * backend.
- */
- do
- {
-#define PARAMS_ARRAY_SIZE 7
-
- const char *keywords[PARAMS_ARRAY_SIZE];
- const char *values[PARAMS_ARRAY_SIZE];
-
- keywords[0] = "host";
- values[0] = pghost;
- keywords[1] = "port";
- values[1] = pgport;
- keywords[2] = "user";
- values[2] = login;
- keywords[3] = "password";
- values[3] = have_password ? password : NULL;
- keywords[4] = "dbname";
- values[4] = dbName;
- keywords[5] = "fallback_application_name";
- values[5] = progname;
- keywords[6] = NULL;
- values[6] = NULL;
-
- new_pass = false;
-
- conn = PQconnectdbParams(keywords, values, true);
-
- if (!conn)
- {
- fprintf(stderr, "connection to database \"%s\" failed\n",
- dbName);
- return NULL;
- }
-
- if (PQstatus(conn) == CONNECTION_BAD &&
- PQconnectionNeedsPassword(conn) &&
- !have_password)
- {
- PQfinish(conn);
- simple_prompt("Password: ", password, sizeof(password), false);
- have_password = true;
- new_pass = true;
- }
- } while (new_pass);
-
- /* check to see that the backend connection was successfully made */
- if (PQstatus(conn) == CONNECTION_BAD)
- {
- fprintf(stderr, "connection to database \"%s\" failed:\n%s",
- dbName, PQerrorMessage(conn));
- PQfinish(conn);
- return NULL;
- }
+ /* Connect to database, with optional password prompting */
+ conn = connect_with_password_prompt(pghost,
+ pgport,
+ login,
+ dbName,
+ progname,
+ NULL,
+ true);
return conn;
}
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index b82d3f65c4..8357903aa3 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -78,7 +78,7 @@ pgbench(
1,
[qr{^$}],
[
- qr{connection to database "no-such-database" failed},
+ qr{could not connect to database "no-such-database"},
qr{FATAL: database "no-such-database" does not exist}
],
'no such database');
diff --git a/src/bin/scripts/common.c b/src/bin/scripts/common.c
index 1b38a1da49..aed0af740d 100644
--- a/src/bin/scripts/common.c
+++ b/src/bin/scripts/common.c
@@ -74,7 +74,6 @@ connectDatabase(const char *dbname, const char *pghost,
bool echo, bool fail_ok, bool allow_password_reuse)
{
PGconn *conn;
- bool new_pass;
static bool have_password = false;
static char password[100];
@@ -87,64 +86,19 @@ connectDatabase(const char *dbname, const char *pghost,
have_password = true;
}
- /*
- * Start the connection. Loop until we have a password if requested by
- * backend.
- */
- do
- {
- const char *keywords[7];
- const char *values[7];
-
- keywords[0] = "host";
- values[0] = pghost;
- keywords[1] = "port";
- values[1] = pgport;
- keywords[2] = "user";
- values[2] = pguser;
- keywords[3] = "password";
- values[3] = have_password ? password : NULL;
- keywords[4] = "dbname";
- values[4] = dbname;
- keywords[5] = "fallback_application_name";
- values[5] = progname;
- keywords[6] = NULL;
- values[6] = NULL;
-
- new_pass = false;
- conn = PQconnectdbParams(keywords, values, true);
-
- if (!conn)
- {
- pg_log_error("could not connect to database %s: out of memory",
- dbname);
- exit(1);
- }
-
- /*
- * No luck? Trying asking (again) for a password.
- */
- if (PQstatus(conn) == CONNECTION_BAD &&
- PQconnectionNeedsPassword(conn) &&
- prompt_password != TRI_NO)
- {
- PQfinish(conn);
- simple_prompt("Password: ", password, sizeof(password), false);
- have_password = true;
- new_pass = true;
- }
- } while (new_pass);
-
- /* check to see that the backend connection was successfully made */
- if (PQstatus(conn) == CONNECTION_BAD)
+ /* grab a connection, with password prompting */
+ conn = connect_with_password_prompt(pghost,
+ pgport,
+ pguser,
+ dbname,
+ progname,
+ password,
+ prompt_password != TRI_NO);
+ if (conn == NULL)
{
+ /* an error has been generated */
if (fail_ok)
- {
- PQfinish(conn);
return NULL;
- }
- pg_log_error("could not connect to database %s: %s",
- dbname, PQerrorMessage(conn));
exit(1);
}
diff --git a/src/fe_utils/Makefile b/src/fe_utils/Makefile
index 7d73800323..670eea94e1 100644
--- a/src/fe_utils/Makefile
+++ b/src/fe_utils/Makefile
@@ -19,7 +19,8 @@ include $(top_builddir)/src/Makefile.global
override CPPFLAGS := -DFRONTEND -I$(libpq_srcdir) $(CPPFLAGS)
-OBJS = mbprint.o print.o psqlscan.o simple_list.o string_utils.o conditional.o
+OBJS = connect.o conditional.o mbprint.o print.o psqlscan.o \
+ simple_list.o string_utils.o
all: libpgfeutils.a
diff --git a/src/fe_utils/connect.c b/src/fe_utils/connect.c
new file mode 100644
index 0000000000..cae69f1f9a
--- /dev/null
+++ b/src/fe_utils/connect.c
@@ -0,0 +1,112 @@
+/*-------------------------------------------------------------------------
+ *
+ * Utility functions to connect to a backend.
+ *
+ *
+ * Portions Copyright (c) 1996-2019, PostgreSQL Global Development Group
+ * Portions Copyright (c) 1994, Regents of the University of California
+ *
+ * src/fe_utils/connect.c
+ *
+ *-------------------------------------------------------------------------
+ */
+
+#include "postgres_fe.h"
+
+#include "common/logging.h"
+#include "fe_utils/connect.h"
+
+#include "libpq-fe.h"
+
+/*
+ * connect_with_password_prompt
+ *
+ * Connect to a PostgreSQL backend with the given set of connection
+ * parameters, optionally asking for password prompting.
+ *
+ * The caller can optionally pass a password with "saved_password"
+ * which will be used at the first connection attempt.
+ *
+ * The result is the connection to the backend. On error, the result
+ * is NULL and an error is logged.
+ */
+PGconn *
+connect_with_password_prompt(const char *hostname,
+ const char *port,
+ const char *username,
+ const char *dbname,
+ const char *progname,
+ char *saved_password,
+ bool enable_prompt)
+{
+ PGconn *conn;
+ bool new_pass;
+ static bool have_password = false;
+ static char password[100];
+
+ if (saved_password != NULL)
+ {
+ /* XXX: need to be more careful with overflows here */
+ memcpy(password, saved_password, strlen(saved_password) + 1);
+ have_password = true;
+ }
+
+ /*
+ * Start the connection. Loop until we have a password if requested by
+ * backend.
+ */
+ do
+ {
+#define PARAMS_ARRAY_SIZE 7
+
+ const char *keywords[PARAMS_ARRAY_SIZE];
+ const char *values[PARAMS_ARRAY_SIZE];
+
+ keywords[0] = "host";
+ values[0] = hostname;
+ keywords[1] = "port";
+ values[1] = port;
+ keywords[2] = "user";
+ values[2] = username;
+ keywords[3] = "password";
+ values[3] = have_password ? password : NULL;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
+ new_pass = false;
+ conn = PQconnectdbParams(keywords, values, true);
+
+ if (!conn)
+ {
+ pg_log_error("could not connect to database \"%s\"",
+ dbname);
+ return NULL;
+ }
+
+ if (PQstatus(conn) == CONNECTION_BAD &&
+ PQconnectionNeedsPassword(conn) &&
+ !have_password &&
+ enable_prompt)
+ {
+ PQfinish(conn);
+ simple_prompt("Password: ", password, sizeof(password), false);
+ have_password = true;
+ new_pass = true;
+ }
+ } while (new_pass);
+
+ /* check to see that the backend connection was successfully made */
+ if (PQstatus(conn) == CONNECTION_BAD)
+ {
+ pg_log_error("could not connect to database \"%s\": %s",
+ dbname, PQerrorMessage(conn));
+ PQfinish(conn);
+ return NULL;
+ }
+
+ return conn;
+}
diff --git a/src/include/fe_utils/connect.h b/src/include/fe_utils/connect.h
index c1c8c04ab0..fef5d1a1b6 100644
--- a/src/include/fe_utils/connect.h
+++ b/src/include/fe_utils/connect.h
@@ -13,6 +13,8 @@
#ifndef CONNECT_H
#define CONNECT_H
+#include "libpq-fe.h"
+
/*
* This SQL statement installs an always-secure search path, so malicious
* users can't take control. CREATE of an unqualified name will fail, because
@@ -25,4 +27,12 @@
#define ALWAYS_SECURE_SEARCH_PATH_SQL \
"SELECT pg_catalog.set_config('search_path', '', false);"
+extern PGconn * connect_with_password_prompt(const char *hostname,
+ const char *port,
+ const char *username,
+ const char *dbname,
+ const char *progname,
+ char *saved_password,
+ bool enable_prompt);
+
#endif /* CONNECT_H */
diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm
index 2eab635898..141a7e20cb 100644
--- a/src/tools/msvc/Mkvcbuild.pm
+++ b/src/tools/msvc/Mkvcbuild.pm
@@ -141,7 +141,8 @@ sub mkvcbuild
our @pgcommonbkndfiles = @pgcommonallfiles;
our @pgfeutilsfiles = qw(
- conditional.c mbprint.c print.c psqlscan.l psqlscan.c simple_list.c string_utils.c);
+ conditional.c connect.c mbprint.c print.c psqlscan.l psqlscan.c
+ simple_list.c string_utils.c);
$libpgport = $solution->AddProject('libpgport', 'lib', 'misc');
$libpgport->AddDefine('FRONTEND');
signature.asc
Description: PGP signature
