Fix handling of -d "connection string" in pg_dump/pg_restore.

Parallel pg_dump failed if its -d parameter was a connection string
containing any essential information other than host, port, or username.
The same was true for pg_restore with --create.

The reason is that these scenarios failed to preserve the connection
string from the command line; the code felt free to replace that with
just the database name when reconnecting from a pg_dump parallel worker
or after creating the target database.  By chance, parallel pg_restore
did not suffer this defect, as long as you didn't say --create.

In practice it seems that the error would be obvious only if the
connstring included essential, non-default SSL or GSS parameters.
This may explain why it took us so long to notice.  (It also makes
it very difficult to craft a regression test case illustrating the
problem, since the test would fail in builds without those options.)

Fix by refactoring so that ConnectDatabase always receives all the
relevant options directly from the command line, rather than
reconstructed values.  Inject a different database name, when necessary,
by relying on libpq's rules for handling multiple "dbname" parameters.

While here, let's get rid of the essentially duplicate _connectDB
function, as well as some obsolete nearby cruft.

Per bug #16604 from Zsolt Ero.  Back-patch to all supported branches.

Discussion: https://postgr.es/m/16604-933f4b8791227b15@postgresql.org

1 files changed, 66 insertions, 171 deletions

diff --git a/src/bin/pg_dump/pg_backup_db.c b/src/bin/pg_dump/pg_backup_db.c
index 12899e26e29..5ba43441f50 100644
--- a/src/bin/pg_dump/pg_backup_db.c
+++ b/src/bin/pg_dump/pg_backup_db.c
@@ -27,7 +27,6 @@
 #include "pg_backup_utils.h"
 
 static void _check_database_version(ArchiveHandle *AH);
-static PGconn *_connectDB(ArchiveHandle *AH, const char *newdbname, const char *newUser);
 static void notice_processor(void *arg, const char *message);
 
 static void
@@ -73,211 +72,100 @@ _check_database_version(ArchiveHandle *AH)
 
 /*
  * Reconnect to the server.  If dbname is not NULL, use that database,
- * else the one associated with the archive handle.  If username is
- * not NULL, use that user name, else the one from the handle.
+ * else the one associated with the archive handle.
  */
 void
-ReconnectToServer(ArchiveHandle *AH, const char *dbname, const char *username)
+ReconnectToServer(ArchiveHandle *AH, const char *dbname)
 {
-	PGconn	   *newConn;
-	const char *newdbname;
-	const char *newusername;
-
-	if (!dbname)
-		newdbname = PQdb(AH->connection);
-	else
-		newdbname = dbname;
-
-	if (!username)
-		newusername = PQuser(AH->connection);
-	else
-		newusername = username;
-
-	newConn = _connectDB(AH, newdbname, newusername);
-
-	/* Update ArchiveHandle's connCancel before closing old connection */
-	set_archive_cancel_info(AH, newConn);
-
-	PQfinish(AH->connection);
-	AH->connection = newConn;
-
-	/* Start strict; later phases may override this. */
-	PQclear(ExecuteSqlQueryForSingleRow((Archive *) AH,
-										ALWAYS_SECURE_SEARCH_PATH_SQL));
-}
-
-/*
- * Connect to the db again.
- *
- * Note: it's not really all that sensible to use a single-entry password
- * cache if the username keeps changing.  In current usage, however, the
- * username never does change, so one savedPassword is sufficient.  We do
- * update the cache on the off chance that the password has changed since the
- * start of the run.
- */
-static PGconn *
-_connectDB(ArchiveHandle *AH, const char *reqdb, const char *requser)
-{
-	PQExpBufferData connstr;
-	PGconn	   *newConn;
-	const char *newdb;
-	const char *newuser;
-	char	   *password;
-	bool		new_pass;
-
-	if (!reqdb)
-		newdb = PQdb(AH->connection);
-	else
-		newdb = reqdb;
-
-	if (!requser || strlen(requser) == 0)
-		newuser = PQuser(AH->connection);
-	else
-		newuser = requser;
-
-	pg_log_info("connecting to database \"%s\" as user \"%s\"",
-				newdb, newuser);
-
-	password = AH->savedPassword;
-
-	if (AH->promptPassword == TRI_YES && password == NULL)
-		password = simple_prompt("Password: ", false);
-
-	initPQExpBuffer(&connstr);
-	appendPQExpBufferStr(&connstr, "dbname=");
-	appendConnStrVal(&connstr, newdb);
-
-	do
-	{
-		const char *keywords[7];
-		const char *values[7];
-
-		keywords[0] = "host";
-		values[0] = PQhost(AH->connection);
-		keywords[1] = "port";
-		values[1] = PQport(AH->connection);
-		keywords[2] = "user";
-		values[2] = newuser;
-		keywords[3] = "password";
-		values[3] = password;
-		keywords[4] = "dbname";
-		values[4] = connstr.data;
-		keywords[5] = "fallback_application_name";
-		values[5] = progname;
-		keywords[6] = NULL;
-		values[6] = NULL;
-
-		new_pass = false;
-		newConn = PQconnectdbParams(keywords, values, true);
-
-		if (!newConn)
-			fatal("could not reconnect to database");
-
-		if (PQstatus(newConn) == CONNECTION_BAD)
-		{
-			if (!PQconnectionNeedsPassword(newConn))
-				fatal("could not reconnect to database: %s",
-					  PQerrorMessage(newConn));
-			PQfinish(newConn);
-
-			if (password)
-				fprintf(stderr, "Password incorrect\n");
-
-			fprintf(stderr, "Connecting to %s as %s\n",
-					newdb, newuser);
-
-			if (AH->promptPassword != TRI_NO)
-			{
-				if (password && password != AH->savedPassword)
-					free(password);
-				password = simple_prompt("Password: ", false);
-			}
-			else
-				fatal("connection needs password");
-
-			new_pass = true;
-		}
-	} while (new_pass);
-
-	if (password && password != AH->savedPassword)
-		free(password);
+	PGconn	   *oldConn = AH->connection;
+	RestoreOptions *ropt = AH->public.ropt;
 
 	/*
-	 * We want to remember connection's actual password, whether or not we got
-	 * it by prompting.  So we don't just store the password variable.
+	 * Save the dbname, if given, in override_dbname so that it will also
+	 * affect any later reconnection attempt.
 	 */
-	if (PQconnectionUsedPassword(newConn))
-	{
-		if (AH->savedPassword)
-			free(AH->savedPassword);
-		AH->savedPassword = pg_strdup(PQpass(newConn));
-	}
-
-	termPQExpBuffer(&connstr);
+	if (dbname)
+		ropt->cparams.override_dbname = pg_strdup(dbname);
 
-	/* check for version mismatch */
-	_check_database_version(AH);
+	/*
+	 * Note: we want to establish the new connection, and in particular update
+	 * ArchiveHandle's connCancel, before closing old connection.  Otherwise
+	 * an ill-timed SIGINT could try to access a dead connection.
+	 */
+	AH->connection = NULL;		/* dodge error check in ConnectDatabase */
 
-	PQsetNoticeProcessor(newConn, notice_processor, NULL);
+	ConnectDatabase((Archive *) AH, &ropt->cparams, true);
 
-	return newConn;
+	PQfinish(oldConn);
 }
 
-
 /*
- * Make a database connection with the given parameters.  The
- * connection handle is returned, the parameters are stored in AHX.
- * An interactive password prompt is automatically issued if required.
+ * Make, or remake, a database connection with the given parameters.
+ *
+ * The resulting connection handle is stored in AHX->connection.
  *
+ * An interactive password prompt is automatically issued if required.
+ * We store the results of that in AHX->savedPassword.
  * Note: it's not really all that sensible to use a single-entry password
  * cache if the username keeps changing.  In current usage, however, the
  * username never does change, so one savedPassword is sufficient.
  */
 void
 ConnectDatabase(Archive *AHX,
-				const char *dbname,
-				const char *pghost,
-				const char *pgport,
-				const char *username,
-				trivalue prompt_password)
+				const ConnParams *cparams,
+				bool isReconnect)
 {
 	ArchiveHandle *AH = (ArchiveHandle *) AHX;
+	trivalue	prompt_password;
 	char	   *password;
 	bool		new_pass;
 
 	if (AH->connection)
 		fatal("already connected to a database");
 
+	/* Never prompt for a password during a reconnection */
+	prompt_password = isReconnect ? TRI_NO : cparams->promptPassword;
+
 	password = AH->savedPassword;
 
 	if (prompt_password == TRI_YES && password == NULL)
 		password = simple_prompt("Password: ", false);
 
-	AH->promptPassword = prompt_password;
-
 	/*
 	 * 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] = 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;
+		const char *keywords[8];
+		const char *values[8];
+		int			i = 0;
+
+		/*
+		 * If dbname is a connstring, its entries can override the other
+		 * values obtained from cparams; but in turn, override_dbname can
+		 * override the dbname component of it.
+		 */
+		keywords[i] = "host";
+		values[i++] = cparams->pghost;
+		keywords[i] = "port";
+		values[i++] = cparams->pgport;
+		keywords[i] = "user";
+		values[i++] = cparams->username;
+		keywords[i] = "password";
+		values[i++] = password;
+		keywords[i] = "dbname";
+		values[i++] = cparams->dbname;
+		if (cparams->override_dbname)
+		{
+			keywords[i] = "dbname";
+			values[i++] = cparams->override_dbname;
+		}
+		keywords[i] = "fallback_application_name";
+		values[i++] = progname;
+		keywords[i] = NULL;
+		values[i++] = NULL;
+		Assert(i <= lengthof(keywords));
 
 		new_pass = false;
 		AH->connection = PQconnectdbParams(keywords, values, true);
@@ -298,9 +186,16 @@ ConnectDatabase(Archive *AHX,
 
 	/* 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));
+	{
+		if (isReconnect)
+			fatal("reconnection to database \"%s\" failed: %s",
+				  PQdb(AH->connection) ? PQdb(AH->connection) : "",
+				  PQerrorMessage(AH->connection));
+		else
+			fatal("connection to database \"%s\" failed: %s",
+				  PQdb(AH->connection) ? PQdb(AH->connection) : "",
+				  PQerrorMessage(AH->connection));
+	}
 
 	/* Start strict; later phases may override this. */
 	PQclear(ExecuteSqlQueryForSingleRow((Archive *) AH,