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

I wrote:
> I did actually look into saving the active connection's PQconninfo
> immediately at connection establishment and then referring to it in any
> subsequent \connect.  Then things could work the same even if the original
> connection had failed meanwhile.  But there are technical details that
> make that a lot harder than it seems on the surface --- mainly, that the
> way do_connect works now requires that it have a copy of the PQconninfo
> data that it can scribble on, and that won't do if we need the saved
> PQconninfo to persist when a \connect attempt fails.  That could be dealt
> with with enough new code, but I didn't think it was worth the trouble.

Actually ... I'd no sooner pushed that patch than I realized that there
*is* an easy, if rather grotty, way to deal with this.  We can just not
issue PQfinish on the old/dead connection until we've successfully made
a new one.  PQconninfo doesn't care if the connection is in BAD state.

To avoid introducing weird behaviors, we can't keep the logically-dead
connection in pset.db, but introducing a separate variable to hold such
a connection doesn't seem too awful.  So that leads me to the attached
patch, which is able to reconnect even if we lost the connection:

regression=# select 1;
 ?column?
----------
        1
(1 row)

-- in another window, stop the server, then:

regression=# select 1;
FATAL:  terminating connection due to administrator command
server closed the connection unexpectedly
        This probably means the server terminated abnormally
        before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

--- now restart the server, and:

!?> \c
You are now connected to database "regression" as user "postgres" via socket in "/tmp" at port "5432".

I would not have wanted to accept a patch that did it the other way,
because it would have been a mess, but this seems small enough to
be worth doing.  The only real objection I can see is that it could
hold a server connection open when the user thinks there is none;
but that could only happen in a non-interactive script, and it does
not seem like a big problem in that case.  We could alternatively
not stash the "dead" connection after a non-interactive \connect
failure, but I doubt that's better.

            regards, tom lane

diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 39a460d85c..a6b1d7533d 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -3060,26 +3060,29 @@ 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,
+     * old connection, then replace individual values as necessary.  (We may
+     * need to resort to looking at pset.dead_conn, if the connection died
+     * previously or a previous non-interactive \connect failed.)  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 should be unreachable, but just in case ... */
+            pg_log_error("No database connection exists to re-use parameters from");
+            return false;
+        }
+    }
     else
         cinfo = PQconndefaults();
 
@@ -3360,10 +3363,14 @@ 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 live connection; but stash away the
+                 * previous connection so that we can still refer to its
+                 * parameters in a later \connect attempt.  Keep this bit in
+                 * sync with CheckConnection().
                  */
-                PQfinish(o_conn);
+                if (pset.dead_conn)
+                    PQfinish(pset.dead_conn);
+                pset.dead_conn = o_conn;
                 pset.db = NULL;
                 ResetCancelConn();
                 UnsyncVariables();
@@ -3421,8 +3428,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
index 6323a35c91..80fcd6a0d6 100644
--- 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
+             * 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 this bit 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/settings.h b/src/bin/psql/settings.h
index 97941aa10c..9601f6e90c 100644
--- 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
index 8232a0143b..e8d35a108f 100644
--- 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 @@ error:
     /* 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;