Mop-up around psql's \connect behavior - Mailing list pgsql-hackers

While working on commit 85c54287a, I noticed a few things I did not
much care for in do_connect().  These don't quite seem to rise to
the level of back-patchable bugs, but they're still not great:

* The initial stanza that complains about

    if (!o_conn && (!dbname || !user || !host || !port))

seems woefully obsolete.  In the first place, it's pretty silly
to equate a "complete connection specification" with having just
those four values; the whole point of 85c54287a and predecessors
is that other settings such as sslmode may be just as important.
In the second place, this fails to consider the possibility that
we only have a connstring parameter --- which may nonetheless
provide all the required settings.  And in the third place,
this clearly wasn't revisited when we added explicit control of
whether or not we're supposed to re-use parameters from the old
connection.  It's very silly to insist on having an o_conn if we're
going to ignore it anyway.

I think the reason we've not had complaints about this is that the
situation normally doesn't arise in interactive sessions (since we
won't release the old connection voluntarily), while scripts are
likely not designed to cope with connection losses anyway.  These
facts militate against spending a whole lot of effort on a fix,
but still we ought to reduce the silliness factor.  What I propose
is to complain if we have no o_conn *and* we are asked to re-use
parameters from it.  Otherwise, it's fine.

* I really don't like the bit about silently ignoring user, host,
and port parameters if we see that the first parameter is a connstring.
That's as user-unfriendly as can be.  It should be a syntax error
to specify both; the documentation certainly implies that it is.

* The old-style-syntax code path understands that it should re-use
the old password (if any) when the user, host, and port settings
haven't changed.  The connstring code path was too lazy to make
that work, but now that we're deconstructing the connstring there's
very little excuse for not having it act the same way.

The attached patch fixes these things and documents the password
behavior, which for some reason went unmentioned before.  Along
the way I simplified the mechanism for re-using a password a bit;
there's no reason to treat it so much differently from re-using
other parameters.

Any objections?

            regards, tom lane

diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 7d0d361657..f6f77dbac3 100644
--- 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 ba3ea39aaa..39a460d85c 100644
--- 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;