Allow psql to re-use connection parameters after a connection loss.

tglsfdc · tglsfdc · commit 1b62d0fb3e50 · 2020-10-23T17:07:15.000-04:00
Instead of immediately PQfinish'ing a dead connection, save it aside so that we can still extract its parameters for \connect attempts. (This works because PQconninfo doesn't care whether the PGconn is in CONNECTION_BAD state.) This allows developers to reconnect with just \c after a database crash and restart. It's tempting to use the same approach instead of closing the old connection after a failed non-interactive \connect command. However, that would not be very safe: consider a script containing \c db1 user1 live_server \c db2 user2 dead_server \c db3 The script would be expecting to connect to db3 at dead_server, but if we re-use parameters from the first connection then it might successfully connect to db3 at live_server. This'd defeat the goal of not letting a script accidentally execute commands against the wrong database. Discussion: https://postgr.es/m/38464.1603394584@sss.pgh.pa.us
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
@@ -931,12 +931,23 @@ testdb=&gt;
         connection is closed.
         If the connection attempt fails (wrong user name, access
         denied, etc.), the previous connection will be kept if
-        <application>psql</application> is in interactive mode. But when
-        executing a non-interactive script, processing will
-        immediately stop with an error. This distinction was chosen as
+        <application>psql</application> is in interactive mode.  But when
+        executing a non-interactive script, the old connection is closed
+        and an error is reported.  That may or may not terminate the
+        script; if it does not, all database-accessing commands will fail
+        until another <literal>\connect</literal> command is successfully
+        executed.  This distinction was chosen as
         a user convenience against typos on the one hand, and a safety
         mechanism that scripts are not accidentally acting on the
         wrong database on the other hand.
+        Note that whenever a <literal>\connect</literal> command attempts
+        to re-use parameters, the values re-used are those of the last
+        successful connection, not of any failed attempts made subsequently.
+        However, in the case of a
+        non-interactive <literal>\connect</literal> failure, no parameters
+        are allowed to be re-used later, since the script would likely be
+        expecting the values from the failed <literal>\connect</literal>
+        to be re-used.
         </para>
 
         <para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
@@ -3060,26 +3060,28 @@ do_connect(enum trivalue reuse_previous_specification,
 			break;
 	}
 
-	/*
-	 * If we are to re-use parameters, there'd better be an old connection to
-	 * get them from.
-	 */
-	if (reuse_previous && !o_conn)
-	{
-		pg_log_error("No database connection exists to re-use parameters from");
-		return false;
-	}
-
 	/*
 	 * If we intend to re-use connection parameters, collect them out of the
-	 * old connection, then replace individual values as necessary. Otherwise,
-	 * obtain a PQconninfoOption array containing libpq's defaults, and modify
-	 * that.  Note this function assumes that PQconninfo, PQconndefaults, and
-	 * PQconninfoParse will all produce arrays containing the same options in
-	 * the same order.
+	 * old connection, then replace individual values as necessary.  (We may
+	 * need to resort to looking at pset.dead_conn, if the connection died
+	 * previously.)  Otherwise, obtain a PQconninfoOption array containing
+	 * libpq's defaults, and modify that.  Note this function assumes that
+	 * PQconninfo, PQconndefaults, and PQconninfoParse will all produce arrays
+	 * containing the same options in the same order.
 	 */
 	if (reuse_previous)
-		cinfo = PQconninfo(o_conn);
+	{
+		if (o_conn)
+			cinfo = PQconninfo(o_conn);
+		else if (pset.dead_conn)
+			cinfo = PQconninfo(pset.dead_conn);
+		else
+		{
+			/* This is reachable after a non-interactive \connect failure */
+			pg_log_error("No database connection exists to re-use parameters from");
+			return false;
+		}
+	}
 	else
 		cinfo = PQconndefaults();
 
@@ -3360,14 +3362,26 @@ do_connect(enum trivalue reuse_previous_specification,
 			if (o_conn)
 			{
 				/*
-				 * Transition to having no connection.  Keep this bit in sync
-				 * with CheckConnection().
+				 * Transition to having no connection.
+				 *
+				 * Unlike CheckConnection(), we close the old connection
+				 * immediately to prevent its parameters from being re-used.
+				 * This is so that a script cannot accidentally reuse
+				 * parameters it did not expect to.  Otherwise, the state
+				 * cleanup should be the same as in CheckConnection().
 				 */
 				PQfinish(o_conn);
 				pset.db = NULL;
 				ResetCancelConn();
 				UnsyncVariables();
 			}
+
+			/* On the same reasoning, release any dead_conn to prevent reuse */
+			if (pset.dead_conn)
+			{
+				PQfinish(pset.dead_conn);
+				pset.dead_conn = NULL;
+			}
 		}
 
 		return false;
@@ -3421,8 +3435,15 @@ do_connect(enum trivalue reuse_previous_specification,
 				   PQdb(pset.db), PQuser(pset.db));
 	}
 
+	/* Drop no-longer-needed connection(s) */
 	if (o_conn)
 		PQfinish(o_conn);
+	if (pset.dead_conn)
+	{
+		PQfinish(pset.dead_conn);
+		pset.dead_conn = NULL;
+	}
+
 	return true;
 }
 
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
@@ -313,10 +313,14 @@ CheckConnection(void)
 			fprintf(stderr, _("Failed.\n"));
 
 			/*
-			 * Transition to having no connection.  Keep this bit in sync with
-			 * do_connect().
+			 * Transition to having no connection; but stash away the failed
+			 * connection so that we can still refer to its parameters in a
+			 * later \connect attempt.  Keep the state cleanup here in sync
+			 * with do_connect().
 			 */
-			PQfinish(pset.db);
+			if (pset.dead_conn)
+				PQfinish(pset.dead_conn);
+			pset.dead_conn = pset.db;
 			pset.db = NULL;
 			ResetCancelConn();
 			UnsyncVariables();
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
@@ -2744,7 +2744,7 @@ describeOneTableDetails(const char *schemaname,
 					/* Show the stats target if it's not default */
 					if (strcmp(PQgetvalue(result, i, 8), "-1") != 0)
 						appendPQExpBuffer(&buf, "; STATISTICS %s",
-									  PQgetvalue(result, i, 8));
+										  PQgetvalue(result, i, 8));
 
 					printTableAddFooter(&cont, buf.data);
 				}
diff --git a/src/bin/psql/settings.h b/src/bin/psql/settings.h
@@ -117,6 +117,13 @@ typedef struct _psqlSettings
 
 	VariableSpace vars;			/* "shell variable" repository */
 
+	/*
+	 * If we get a connection failure, the now-unusable PGconn is stashed here
+	 * until we can successfully reconnect.  Never attempt to do anything with
+	 * this PGconn except extract parameters for a \connect attempt.
+	 */
+	PGconn	   *dead_conn;		/* previous connection to backend */
+
 	/*
 	 * The remaining fields are set by assign hooks associated with entries in
 	 * "vars".  They should not be set directly except by those hook
diff --git a/src/bin/psql/startup.c b/src/bin/psql/startup.c
@@ -145,6 +145,7 @@ main(int argc, char *argv[])
 	pset.progname = get_progname(argv[0]);
 
 	pset.db = NULL;
+	pset.dead_conn = NULL;
 	setDecimalLocale();
 	pset.encoding = PQenv2encoding();
 	pset.queryFout = stdout;
@@ -442,7 +443,10 @@ main(int argc, char *argv[])
 	/* clean up */
 	if (pset.logfile)
 		fclose(pset.logfile);
-	PQfinish(pset.db);
+	if (pset.db)
+		PQfinish(pset.db);
+	if (pset.dead_conn)
+		PQfinish(pset.dead_conn);
 	setQFout(NULL);
 
 	return successResult;

Original file line number	Diff line number	Diff line change
`@@ -2744,7 +2744,7 @@ describeOneTableDetails(const char *schemaname,`
`2744`	`2744`	`/* Show the stats target if it's not default */`
`2745`	`2745`	`if (strcmp(PQgetvalue(result, i, 8), "-1") != 0)`
`2746`	`2746`	`appendPQExpBuffer(&buf, "; STATISTICS %s",`
`2747`		`- PQgetvalue(result, i, 8));`
	`2747`	`+ PQgetvalue(result, i, 8));`
`2748`	`2748`
`2749`	`2749`	`printTableAddFooter(&cont, buf.data);`
`2750`	`2750`	`}`