mirrors
/
postgres
mirror of https://github.com/postgres/postgres
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Clean up some unpleasant behaviors in psql's \connect command. 

The check for whether to complain about not having an old connection
to get parameters from was seriously out of date: it had not been
rethought when we invented connstrings, nor when we invented the
-reuse-previous option.  Replace it with a check that throws an
error if reuse-previous is active and we lack an old connection to
reuse.  While that doesn't move the goalposts very far in terms of
easing reconnection after a server crash, at least it's consistent.

If the user specifies a connstring plus additional parameters
(which is invalid per the documentation), the extra parameters were
silently ignored.  That seems like it could be really confusing,
so let's throw a syntax error instead.

Teach the connstring code path to re-use the old connection's password
in the same cases as the old-style-syntax code path would, ie if we
are reusing parameters and the values of username, host/hostaddr, and
port are not being changed.  Document this behavior, too, since it was
unmentioned before.  Also simplify the implementation a bit, giving
rise to two new and useful properties: if there's a "password=xxx" in
the connstring, we'll use it not ignore it, and by default (i.e.,
except with --no-password) we will prompt for a password if the
re-used password or connstring password doesn't work.  The previous
code just failed if the re-used password didn't work.

Given the paucity of field complaints about these issues, I don't
think that they rise to the level of back-patchable bug fixes,
and in any case they might represent undesirable behavior changes
in minor releases.  So no back-patch.

Discussion: https://postgr.es/m/235210.1603321144@sss.pgh.pa.us
This commit is contained in:
Tom Lane 2020-10-22 14:04:21 -04:00
parent 866e24d47d
commit 94929f1cf6
2 changed files with 54 additions and 39 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
doc/src/sgml/ref/psql-ref.sgml
View File

@ -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>

91
src/bin/psql/command.c
View File

@ -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;