Обсуждение: Mop-up around psql's \connect behavior
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;
On 10/21/20 18:59, Tom Lane wrote: > 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've been getting around it just by saying \c "connstring" . . . which works. It gives me a tiny thrill every time I do it, like I'm getting away with something. Which is why I haven't been complaining. I suppose I wouldn't complain if it were fixed, either. Regards, -Chap
At Wed, 21 Oct 2020 18:59:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > 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. Sounds reasonable. > 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. The reason I haven't complain about this is I don't reconnect by \c after involuntary disconnection. (That is, C-d then psql again:p) But once it got on my mind, it might be strange that just \c or \c -reuse-previous=y doesn't reconnect a broken session. It might be better we reuse the previous connection parameter even if the connection has been lost, but this would be another issue. > * 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. +1 > * 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. +1 (I thought sslmode might affect but that is wrong since cert authenticaion cannot be turned off from command line.) > 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. Looks fine. > Any objections? Nope from me. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Wed, 21 Oct 2020 18:59:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
>> ... 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.
> The reason I haven't complain about this is I don't reconnect by \c
> after involuntary disconnection. (That is, C-d then psql again:p)
Yeah, me too.
> But once it got on my mind, it might be strange that just \c or \c
> -reuse-previous=y doesn't reconnect a broken session. It might be
> better we reuse the previous connection parameter even if the
> connection has been lost, but this would be another issue.
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.
(Note that we developers face the server-crashed scenario a whole lot more
often than normal people ;-), so we probably overrate how useful it'd be
to be able to reconnect in that case.)
>> * 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.
> +1 (I thought sslmode might affect but that is wrong since cert
> authenticaion cannot be turned off from command line.)
Yeah. That could affect whether the server asks for a password at
all, but you have to really stretch to think of cases where it could
result in needing a *different* password. An example perhaps is
where pg_hba.conf is configured to do, say, LDAP auth on SSL connections
and normal password auth on non-SSL, and the LDAP server has a different
password than what is in pg_authid. But that seems like something nobody
could want. Also notice that unlike the previous code, with my patch
do_connect will always (barring --no-password) give you an opportunity
to interactively supply a password, even if we initially reused an
old password and it didn't work. So it seems like this will cope
even if you do have a setup as wacko as that.
regards, tom lane
At Thu, 22 Oct 2020 00:34:20 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > > At Wed, 21 Oct 2020 18:59:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > > But once it got on my mind, it might be strange that just \c or \c > > -reuse-previous=y doesn't reconnect a broken session. It might be > > better we reuse the previous connection parameter even if the > > connection has been lost, but this would be another issue. > > 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. Agreed. > (Note that we developers face the server-crashed scenario a whole lot more > often than normal people ;-), so we probably overrate how useful it'd be > to be able to reconnect in that case.) Agreed^2. > >> * 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. > > > +1 (I thought sslmode might affect but that is wrong since cert > > authenticaion cannot be turned off from command line.) > > Yeah. That could affect whether the server asks for a password at > all, but you have to really stretch to think of cases where it could > result in needing a *different* password. An example perhaps is > where pg_hba.conf is configured to do, say, LDAP auth on SSL connections > and normal password auth on non-SSL, and the LDAP server has a different > password than what is in pg_authid. But that seems like something nobody > could want. Also notice that unlike the previous code, with my patch > do_connect will always (barring --no-password) give you an opportunity > to interactively supply a password, even if we initially reused an > old password and it didn't work. So it seems like this will cope > even if you do have a setup as wacko as that. I thought of that scenarios and conclused as the same. Sounds reasonable. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
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;
At Thu, 22 Oct 2020 15:23:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > 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: Sounds reasonable. > 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". Looks good to me. I'm very happy with the result. > 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. Agreed. Thanks! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes:
> At Thu, 22 Oct 2020 15:23:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in
>> ... 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.
> Agreed. Thanks!
After further thought I decided we *must* do it as per my "alternative"
idea. 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.
So we have to not save the connection after a failed script \connect.
However, it seems OK to save after a connection loss whether we're
in a script or not; that is,
\c db1 user1 server1
...
(connection dies here)
... --- these commands will fail
\c db2
The script will be expecting the second \c to re-use parameters
from the first one, and that will still work as expected.
I went ahead and pushed it after adjusting that.
regards, tom lane
At Fri, 23 Oct 2020 17:12:44 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > Kyotaro Horiguchi <horikyota.ntt@gmail.com> writes: > > At Thu, 22 Oct 2020 15:23:04 -0400, Tom Lane <tgl@sss.pgh.pa.us> wrote in > >> ... 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. > > > Agreed. Thanks! > > After further thought I decided we *must* do it as per my "alternative" > idea. 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. Hmm. True. > So we have to not save the connection after a failed script \connect. Yes, we shouldn't save a connection parameters that haven't made a connection. > However, it seems OK to save after a connection loss whether we're > in a script or not; that is, > > \c db1 user1 server1 > ... > (connection dies here) > ... --- these commands will fail > \c db2 > > The script will be expecting the second \c to re-use parameters > from the first one, and that will still work as expected. Agreed. > I went ahead and pushed it after adjusting that. Thanks! -- Kyotaro Horiguchi NTT Open Source Software Center