Clean up some unpleasant behaviors in psql's \connect command.

The check for whether to complain about not having an old connection
to get parameters from was seriously out of date: it had not been
rethought when we invented connstrings, nor when we invented the
-reuse-previous option.  Replace it with a check that throws an
error if reuse-previous is active and we lack an old connection to
reuse.  While that doesn't move the goalposts very far in terms of
easing reconnection after a server crash, at least it's consistent.

If the user specifies a connstring plus additional parameters
(which is invalid per the documentation), the extra parameters were
silently ignored.  That seems like it could be really confusing,
so let's throw a syntax error instead.

Teach the connstring code path to re-use the old connection's password
in the same cases as the old-style-syntax code path would, ie if we
are reusing parameters and the values of username, host/hostaddr, and
port are not being changed.  Document this behavior, too, since it was
unmentioned before.  Also simplify the implementation a bit, giving
rise to two new and useful properties: if there's a "password=xxx" in
the connstring, we'll use it not ignore it, and by default (i.e.,
except with --no-password) we will prompt for a password if the
re-used password or connstring password doesn't work.  The previous
code just failed if the re-used password didn't work.

Given the paucity of field complaints about these issues, I don't
think that they rise to the level of back-patchable bug fixes,
and in any case they might represent undesirable behavior changes
in minor releases.  So no back-patch.

Discussion: https://postgr.es/m/235210.1603321144@sss.pgh.pa.us

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7d0d3616573fc0f9e2d9ca456fb76203d9fd552e..f6f77dbac3ac24015c4bbe23c2319c20b71418d0 100644 (file)
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -920,6 +920,8 @@ testdb=>
         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
index ba3ea39aaa4df76dfa4f19aded0275fcdf854bec..39a460d85ce945d4527e8892d738f5ce806660d7 100644 (file)
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3014,11 +3014,10 @@ param_is_newly_set(const char *old_val, const char *new_val)
 /*
  * do_connect -- handler for \connect
  *
- * Connects to a database with given parameters. Absent an established
- * connection, all parameters are required. Given -reuse-previous=off or a
- * connection string without -reuse-previous=on, NULL values will pass through
- * to PQconnectdbParams(), so the libpq defaults will be used. Otherwise, NULL
- * values will be replaced with the ones in the current connection.
+ * Connects to a database with given parameters.  If we are told to re-use
+ * parameters, parameters from the previous connection are used where the
+ * command's own options do not supply a value.  Otherwise, libpq defaults
+ * are used.
  *
  * In interactive mode, if connection fails with the given parameters,
  * the old connection will be kept.
@@ -3038,20 +3037,16 @@ do_connect(enum trivalue reuse_previous_specification,
        bool            has_connection_string;
        bool            reuse_previous;
 
-       if (!o_conn && (!dbname || !user || !host || !port))
+       has_connection_string = dbname ?
+               recognized_connection_string(dbname) : false;
+
+       /* Complain if we have additional arguments after a connection string. */
+       if (has_connection_string && (user || host || port))
        {
-               /*
-                * We don't know the supplied connection parameters and don't want to
-                * connect to the wrong database by using defaults, so require all
-                * parameters to be specified.
-                */
-               pg_log_error("All connection parameters must be supplied because no "
-                                        "database connection exists");
+               pg_log_error("Do not give user, host, or port separately when using a connection string");
                return false;
        }
 
-       has_connection_string = dbname ?
-               recognized_connection_string(dbname) : false;
        switch (reuse_previous_specification)
        {
                case TRI_YES:
@@ -3065,16 +3060,14 @@ do_connect(enum trivalue reuse_previous_specification,
                        break;
        }
 
-       /* If the old connection does not exist, there is nothing to reuse. */
-       if (!o_conn)
-               reuse_previous = false;
-
-       /* Silently ignore arguments subsequent to a connection string. */
-       if (has_connection_string)
+       /*
+        * If we are to re-use parameters, there'd better be an old connection to
+        * get them from.
+        */
+       if (reuse_previous && !o_conn)
        {
-               user = NULL;
-               host = NULL;
-               port = NULL;
+               pg_log_error("No database connection exists to re-use parameters from");
+               return false;
        }
 
        /*
@@ -3103,6 +3096,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;
@@ -3121,6 +3115,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);
@@ -3130,8 +3144,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;
@@ -3219,14 +3238,6 @@ 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;
-       }
 
        /* Loop till we have a connection or fail, which we might've already */
        while (success)
@@ -3238,12 +3249,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++)
                {
@@ -3262,7 +3273,9 @@ 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;