Fix recently-introduced breakage in psql's \connect command.

tglsfdc · tglsfdc · commit 777ac03a6823 · 2020-11-29T15:22:04.000-05:00
Through my misreading of what the existing code actually did, commits 85c5428 et al. broke psql's behavior for the case where "\c connstring" provides a password in the connstring. We should use that password in such a case, but as of 85c5428 we ignored it (and instead, prompted for a password). Commit 94929f1 fixed that in HEAD, but since I thought it was cleaning up a longstanding misbehavior and not one I'd just created, I didn't back-patch it. Hence, back-patch the portions of 94929f1 having to do with password management. In addition to fixing the introduced bug, this means that "\c -reuse-previous=on connstring" will allow re-use of an existing connection's password if the connstring doesn't change user/host/port. That didn't happen before, but it seems like a bug fix, and anyway I'm loath to have significant differences in this code across versions. Also fix an error with the same root cause about whether or not to override a connstring's setting of client_encoding. As of 85c5428 we always did so; restore the previous behavior of overriding only when stdin/stdout are a terminal and there's no environment setting of PGCLIENTENCODING. (I find that definition a bit surprising, but right now doesn't seem like the time to revisit it.) Per bug #16746 from Krzysztof Gradek. As with the previous patch, back-patch to all supported branches. Discussion: https://postgr.es/m/16746-44b30e2edf4335d4@postgresql.org
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
@@ -911,6 +911,8 @@ testdb=&gt;
         is changed from its previous value using the positional syntax,
         any <replaceable>hostaddr</replaceable> setting present in the
         existing connection's parameters is dropped.
+        Also, any password used for the existing connection will be re-used
+        only if the user, host, and port settings are not changed.
         When the command neither specifies nor reuses a particular parameter,
         the <application>libpq</application> default is used.
         </para>
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
@@ -2857,6 +2857,7 @@ do_connect(enum trivalue reuse_previous_specification,
 	int			nconnopts = 0;
 	bool		same_host = false;
 	char	   *password = NULL;
+	char	   *client_encoding;
 	bool		success = true;
 	bool		keep_password = true;
 	bool		has_connection_string;
@@ -2927,6 +2928,7 @@ do_connect(enum trivalue reuse_previous_specification,
 			{
 				PQconninfoOption *ci;
 				PQconninfoOption *replci;
+				bool		have_password = false;
 
 				for (ci = cinfo, replci = replcinfo;
 					 ci->keyword && replci->keyword;
@@ -2945,6 +2947,26 @@ do_connect(enum trivalue reuse_previous_specification,
 
 						replci->val = ci->val;
 						ci->val = swap;
+
+						/*
+						 * Check whether connstring provides options affecting
+						 * password re-use.  While any change in user, host,
+						 * hostaddr, or port causes us to ignore the old
+						 * connection's password, we don't force that for
+						 * dbname, since passwords aren't database-specific.
+						 */
+						if (replci->val == NULL ||
+							strcmp(ci->val, replci->val) != 0)
+						{
+							if (strcmp(replci->keyword, "user") == 0 ||
+								strcmp(replci->keyword, "host") == 0 ||
+								strcmp(replci->keyword, "hostaddr") == 0 ||
+								strcmp(replci->keyword, "port") == 0)
+								keep_password = false;
+						}
+						/* Also note whether connstring contains a password. */
+						if (strcmp(replci->keyword, "password") == 0)
+							have_password = true;
 					}
 				}
 				Assert(ci->keyword == NULL && replci->keyword == NULL);
@@ -2954,8 +2976,13 @@ do_connect(enum trivalue reuse_previous_specification,
 
 				PQconninfoFree(replcinfo);
 
-				/* We never re-use a password with a conninfo string. */
-				keep_password = false;
+				/*
+				 * If the connstring contains a password, tell the loop below
+				 * that we may use it, regardless of other settings (i.e.,
+				 * cinfo's password is no longer an "old" password).
+				 */
+				if (have_password)
+					keep_password = true;
 
 				/* Don't let code below try to inject dbname into params. */
 				dbname = NULL;
@@ -3043,14 +3070,16 @@ do_connect(enum trivalue reuse_previous_specification,
 		 */
 		password = prompt_for_password(has_connection_string ? NULL : user);
 	}
-	else if (o_conn && keep_password)
-	{
-		password = PQpass(o_conn);
-		if (password && *password)
-			password = pg_strdup(password);
-		else
-			password = NULL;
-	}
+
+	/*
+	 * Consider whether to force client_encoding to "auto" (overriding
+	 * anything in the connection string).  We do so if we have a terminal
+	 * connection and there is no PGCLIENTENCODING environment setting.
+	 */
+	if (pset.notty || getenv("PGCLIENTENCODING"))
+		client_encoding = NULL;
+	else
+		client_encoding = "auto";
 
 	/* Loop till we have a connection or fail, which we might've already */
 	while (success)
@@ -3062,12 +3091,12 @@ do_connect(enum trivalue reuse_previous_specification,
 
 		/*
 		 * Copy non-default settings into the PQconnectdbParams parameter
-		 * arrays; but override any values specified old-style, as well as the
-		 * password and a couple of fields we want to set forcibly.
+		 * arrays; but inject any values specified old-style, as well as any
+		 * interactively-obtained password, and a couple of fields we want to
+		 * set forcibly.
 		 *
 		 * If you change this code, see also the initial-connection code in
-		 * main().  For no good reason, a connection string password= takes
-		 * precedence in main() but not here.
+		 * main().
 		 */
 		for (ci = cinfo; ci->keyword; ci++)
 		{
@@ -3086,12 +3115,15 @@ do_connect(enum trivalue reuse_previous_specification,
 			}
 			else if (port && strcmp(ci->keyword, "port") == 0)
 				values[paramnum++] = port;
-			else if (strcmp(ci->keyword, "password") == 0)
+			/* If !keep_password, we unconditionally drop old password */
+			else if ((password || !keep_password) &&
+					 strcmp(ci->keyword, "password") == 0)
 				values[paramnum++] = password;
 			else if (strcmp(ci->keyword, "fallback_application_name") == 0)
 				values[paramnum++] = pset.progname;
-			else if (strcmp(ci->keyword, "client_encoding") == 0)
-				values[paramnum++] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto";
+			else if (client_encoding &&
+					 strcmp(ci->keyword, "client_encoding") == 0)
+				values[paramnum++] = client_encoding;
 			else if (ci->val)
 				values[paramnum++] = ci->val;
 			/* else, don't bother making libpq parse this keyword */
