Skip to content

Commit 1b62d0f

Browse files
tglsfdctglsfdc
committed
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
1 parent 860593e commit 1b62d0f

File tree

6 files changed

+73
-26
lines changed

6 files changed

+73
-26
lines changed

‎doc/src/sgml/ref/psql-ref.sgml

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -931,12 +931,23 @@ testdb=>
931931
connection is closed.
932932
If the connection attempt fails (wrong user name, access
933933
denied, etc.), the previous connection will be kept if
934-
<application>psql</application> is in interactive mode. But when
935-
executing a non-interactive script, processing will
936-
immediately stop with an error. This distinction was chosen as
934+
<application>psql</application> is in interactive mode. But when
935+
executing a non-interactive script, the old connection is closed
936+
and an error is reported. That may or may not terminate the
937+
script; if it does not, all database-accessing commands will fail
938+
until another <literal>\connect</literal> command is successfully
939+
executed. This distinction was chosen as
937940
a user convenience against typos on the one hand, and a safety
938941
mechanism that scripts are not accidentally acting on the
939942
wrong database on the other hand.
943+
Note that whenever a <literal>\connect</literal> command attempts
944+
to re-use parameters, the values re-used are those of the last
945+
successful connection, not of any failed attempts made subsequently.
946+
However, in the case of a
947+
non-interactive <literal>\connect</literal> failure, no parameters
948+
are allowed to be re-used later, since the script would likely be
949+
expecting the values from the failed <literal>\connect</literal>
950+
to be re-used.
940951
</para>
941952

942953
<para>

‎src/bin/psql/command.c

Lines changed: 39 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3060,26 +3060,28 @@ do_connect(enum trivalue reuse_previous_specification,
30603060
break;
30613061
}
30623062

3063-
/*
3064-
* If we are to re-use parameters, there'd better be an old connection to
3065-
* get them from.
3066-
*/
3067-
if (reuse_previous && !o_conn)
3068-
{
3069-
pg_log_error("No database connection exists to re-use parameters from");
3070-
return false;
3071-
}
3072-
30733063
/*
30743064
* If we intend to re-use connection parameters, collect them out of the
3075-
* old connection, then replace individual values as necessary. Otherwise,
3076-
* obtain a PQconninfoOption array containing libpq's defaults, and modify
3077-
* that. Note this function assumes that PQconninfo, PQconndefaults, and
3078-
* PQconninfoParse will all produce arrays containing the same options in
3079-
* the same order.
3065+
* old connection, then replace individual values as necessary. (We may
3066+
* need to resort to looking at pset.dead_conn, if the connection died
3067+
* previously.) Otherwise, obtain a PQconninfoOption array containing
3068+
* libpq's defaults, and modify that. Note this function assumes that
3069+
* PQconninfo, PQconndefaults, and PQconninfoParse will all produce arrays
3070+
* containing the same options in the same order.
30803071
*/
30813072
if (reuse_previous)
3082-
cinfo = PQconninfo(o_conn);
3073+
{
3074+
if (o_conn)
3075+
cinfo = PQconninfo(o_conn);
3076+
else if (pset.dead_conn)
3077+
cinfo = PQconninfo(pset.dead_conn);
3078+
else
3079+
{
3080+
/* This is reachable after a non-interactive \connect failure */
3081+
pg_log_error("No database connection exists to re-use parameters from");
3082+
return false;
3083+
}
3084+
}
30833085
else
30843086
cinfo = PQconndefaults();
30853087

@@ -3360,14 +3362,26 @@ do_connect(enum trivalue reuse_previous_specification,
33603362
if (o_conn)
33613363
{
33623364
/*
3363-
* Transition to having no connection. Keep this bit in sync
3364-
* with CheckConnection().
3365+
* Transition to having no connection.
3366+
*
3367+
* Unlike CheckConnection(), we close the old connection
3368+
* immediately to prevent its parameters from being re-used.
3369+
* This is so that a script cannot accidentally reuse
3370+
* parameters it did not expect to. Otherwise, the state
3371+
* cleanup should be the same as in CheckConnection().
33653372
*/
33663373
PQfinish(o_conn);
33673374
pset.db = NULL;
33683375
ResetCancelConn();
33693376
UnsyncVariables();
33703377
}
3378+
3379+
/* On the same reasoning, release any dead_conn to prevent reuse */
3380+
if (pset.dead_conn)
3381+
{
3382+
PQfinish(pset.dead_conn);
3383+
pset.dead_conn = NULL;
3384+
}
33713385
}
33723386

33733387
return false;
@@ -3421,8 +3435,15 @@ do_connect(enum trivalue reuse_previous_specification,
34213435
PQdb(pset.db), PQuser(pset.db));
34223436
}
34233437

3438+
/* Drop no-longer-needed connection(s) */
34243439
if (o_conn)
34253440
PQfinish(o_conn);
3441+
if (pset.dead_conn)
3442+
{
3443+
PQfinish(pset.dead_conn);
3444+
pset.dead_conn = NULL;
3445+
}
3446+
34263447
return true;
34273448
}
34283449

‎src/bin/psql/common.c

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -313,10 +313,14 @@ CheckConnection(void)
313313
fprintf(stderr, _("Failed.\n"));
314314

315315
/*
316-
* Transition to having no connection. Keep this bit in sync with
317-
* do_connect().
316+
* Transition to having no connection; but stash away the failed
317+
* connection so that we can still refer to its parameters in a
318+
* later \connect attempt. Keep the state cleanup here in sync
319+
* with do_connect().
318320
*/
319-
PQfinish(pset.db);
321+
if (pset.dead_conn)
322+
PQfinish(pset.dead_conn);
323+
pset.dead_conn = pset.db;
320324
pset.db = NULL;
321325
ResetCancelConn();
322326
UnsyncVariables();

‎src/bin/psql/describe.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2744,7 +2744,7 @@ describeOneTableDetails(const char *schemaname,
27442744
/* Show the stats target if it's not default */
27452745
if (strcmp(PQgetvalue(result, i, 8), "-1") != 0)
27462746
appendPQExpBuffer(&buf, "; STATISTICS %s",
2747-
PQgetvalue(result, i, 8));
2747+
PQgetvalue(result, i, 8));
27482748

27492749
printTableAddFooter(&cont, buf.data);
27502750
}

‎src/bin/psql/settings.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,13 @@ typedef struct _psqlSettings
117117

118118
VariableSpace vars; /* "shell variable" repository */
119119

120+
/*
121+
* If we get a connection failure, the now-unusable PGconn is stashed here
122+
* until we can successfully reconnect. Never attempt to do anything with
123+
* this PGconn except extract parameters for a \connect attempt.
124+
*/
125+
PGconn *dead_conn; /* previous connection to backend */
126+
120127
/*
121128
* The remaining fields are set by assign hooks associated with entries in
122129
* "vars". They should not be set directly except by those hook

‎src/bin/psql/startup.c

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ main(int argc, char *argv[])
145145
pset.progname = get_progname(argv[0]);
146146

147147
pset.db = NULL;
148+
pset.dead_conn = NULL;
148149
setDecimalLocale();
149150
pset.encoding = PQenv2encoding();
150151
pset.queryFout = stdout;
@@ -442,7 +443,10 @@ main(int argc, char *argv[])
442443
/* clean up */
443444
if (pset.logfile)
444445
fclose(pset.logfile);
445-
PQfinish(pset.db);
446+
if (pset.db)
447+
PQfinish(pset.db);
448+
if (pset.dead_conn)
449+
PQfinish(pset.dead_conn);
446450
setQFout(NULL);
447451

448452
return successResult;

0 commit comments

Comments
 (0)