From 1b62d0fb3e50ede570d0d4e4a2be69d5645b48a7 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 23 Oct 2020 17:07:15 -0400 Subject: [PATCH] Allow psql to re-use connection parameters after a connection loss. 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 --- doc/src/sgml/ref/psql-ref.sgml | 17 ++++++++-- src/bin/psql/command.c | 57 +++++++++++++++++++++++----------- src/bin/psql/common.c | 10 ++++-- src/bin/psql/describe.c | 2 +- src/bin/psql/settings.h | 7 +++++ src/bin/psql/startup.c | 6 +++- 6 files changed, 73 insertions(+), 26 deletions(-) diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml index f6f77dbac3..221a967bfe 100644 --- a/doc/src/sgml/ref/psql-ref.sgml +++ b/doc/src/sgml/ref/psql-ref.sgml @@ -931,12 +931,23 @@ testdb=> connection is closed. If the connection attempt fails (wrong user name, access denied, etc.), the previous connection will be kept if - psql is in interactive mode. But when - executing a non-interactive script, processing will - immediately stop with an error. This distinction was chosen as + psql 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 \connect 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 \connect 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 \connect failure, no parameters + are allowed to be re-used later, since the script would likely be + expecting the values from the failed \connect + to be re-used. diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c index 39a460d85c..c7a83d5dfc 100644 --- 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 index 6323a35c91..ff673665d8 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 - * 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 index 6bb0316bd9..07d640021c 100644 --- 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 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;