rdelaage
/
postgresql
mirror of https://git.postgresql.org/git/postgresql.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix connection string handling in psql's \connect command. 

psql's \connect claims to be able to re-use previous connection
parameters, but in fact it only re-uses the database name, user name,
host name (and possibly hostaddr, depending on version), and port.
This is problematic for assorted use cases.  Notably, pg_dump[all]
emits "\connect databasename" commands which we would like to have
re-use all other parameters.  If such a script is loaded in a psql run
that initially had "-d connstring" with some non-default parameters,
those other parameters would be lost, potentially causing connection
failure.  (Thus, this is the same kind of bug addressed in commits
a45bc8a4f and 8e5793ab6, although the details are much different.)

To fix, redesign do_connect() so that it pulls out all properties
of the old PGconn using PQconninfo(), and then replaces individual
properties in that array.  In the case where we don't wish to re-use
anything, get libpq's default settings using PQconndefaults() and
replace entries in that, so that we don't need different code paths
for the two cases.

This does result in an additional behavioral change for cases where
the original connection parameters allowed multiple hosts, say
"psql -h host1,host2", and the \connect request allows re-use of the
host setting.  Because the previous coding relied on PQhost(), it
would only permit reconnection to the same host originally selected.
Although one can think of scenarios where that's a good thing, there
are others where it is not.  Moreover, that behavior doesn't seem to
meet the principle of least surprise, nor was it documented; nor is
it even clear it was intended, since that coding long pre-dates the
addition of multi-host support to libpq.  Hence, this patch is content
to drop it and re-use the host list as given.

Per Peter Eisentraut's comments on bug #16604.  Back-patch to all
supported branches.

Discussion: https://postgr.es/m/16604-933f4b8791227b15@postgresql.org
This commit is contained in:
Tom Lane 2020-10-21 16:18:40 -04:00
parent 831611b11c
commit 85c54287af
2 changed files with 220 additions and 129 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -889,35 +889,47 @@ testdb=>
<para>
Establishes a new connection to a <productname>PostgreSQL</productname>
server. The connection parameters to use can be specified either
using a positional syntax, or using <replaceable>conninfo</replaceable> connection
strings as detailed in <xref linkend="libpq-connstring"/>.
using a positional syntax (one or more of database name, user,
host, and port), or using a <replaceable>conninfo</replaceable>
connection string as detailed in
<xref linkend="libpq-connstring"/>. If no arguments are given, a
new connection is made using the same parameters as before.
</para>
<para>
Where the command omits database name, user, host, or port, the new
connection can reuse values from the previous connection. By default,
values from the previous connection are reused except when processing
a <replaceable>conninfo</replaceable> string. Passing a first argument
of <literal>-reuse-previous=on</literal>
or <literal>-reuse-previous=off</literal> overrides that default.
When the command neither specifies nor reuses a particular parameter,
the <application>libpq</application> default is used. Specifying any
Specifying any
of <replaceable class="parameter">dbname</replaceable>,
<replaceable class="parameter">username</replaceable>,
<replaceable class="parameter">host</replaceable> or
<replaceable class="parameter">port</replaceable>
as <literal>-</literal> is equivalent to omitting that parameter.
If <literal>hostaddr</literal> was specified in the original
connection's <structname>conninfo</structname>, that address is reused
for the new connection (disregarding any other host specification).
</para>
<para>
The new connection can re-use connection parameters from the previous
connection; not only database name, user, host, and port, but other
settings such as <replaceable>sslmode</replaceable>. By default,
parameters are re-used in the positional syntax, but not when
a <replaceable>conninfo</replaceable> string is given. Passing a
first argument of <literal>-reuse-previous=on</literal>
or <literal>-reuse-previous=off</literal> overrides that default. If
parameters are re-used, then any parameter not explicitly specified as
a positional parameter or in the <replaceable>conninfo</replaceable>
string is taken from the existing connection's parameters. An
exception is that if the <replaceable>host</replaceable> setting
is changed from its previous value using the positional syntax,
any <replaceable>hostaddr</replaceable> setting present in the
existing connection's parameters is dropped.
When the command neither specifies nor reuses a particular parameter,
the <application>libpq</application> default is used.
</para>
<para>
If the new connection is successfully made, the previous
connection is closed.
If the connection attempt failed (wrong user name, access
denied, etc.), the previous connection will only be kept if
<application>psql</application> is in interactive mode. When
If the connection attempt fails (wrong user name, access
denied, etc.), the previous connection will be kept if
<application>psql</application> is in interactive mode. But when
executing a non-interactive script, processing will
immediately stop with an error. This distinction was chosen as
a user convenience against typos on the one hand, and a safety
@ -932,6 +944,7 @@ testdb=&gt;
=&gt; \c mydb myuser host.dom 6432
=&gt; \c service=foo
=&gt; \c "host=localhost port=5432 dbname=mydb connect_timeout=10 sslmode=disable"
=&gt; \c -reuse-previous=on sslmode=require -- changes only sslmode
=&gt; \c postgresql://tom@localhost/mydb?application_name=myapp
</programlisting>
</listitem>

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

@ -3011,26 +3011,6 @@ param_is_newly_set(const char *old_val, const char *new_val)
return false;
}
/* return whether the connection has 'hostaddr' in its conninfo */
static bool
has_hostaddr(PGconn *conn)
{
bool used = false;
PQconninfoOption *ciopt = PQconninfo(conn);
for (PQconninfoOption *p = ciopt; p->keyword != NULL; p++)
{
if (strcmp(p->keyword, "hostaddr") == 0 && p->val != NULL)
{
used = true;
break;
}
}
PQconninfoFree(ciopt);
return used;
}
/*
* do_connect -- handler for \connect
*
@ -3048,13 +3028,15 @@ do_connect(enum trivalue reuse_previous_specification,
char *dbname, char *user, char *host, char *port)
{
PGconn *o_conn = pset.db,
*n_conn;
*n_conn = NULL;
PQconninfoOption *cinfo;
int nconnopts = 0;
bool same_host = false;
char *password = NULL;
char *hostaddr = NULL;
bool keep_password;
bool success = true;
bool keep_password = true;
bool has_connection_string;
bool reuse_previous;
PQExpBufferData connstr;
if (!o_conn && (!dbname || !user || !host || !port))
{
@ -3096,55 +3078,125 @@ do_connect(enum trivalue reuse_previous_specification,
}
/*
* Grab missing values from the old connection. If we grab host (or host
* is the same as before) and hostaddr was set, grab that too.
* 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.
*/
if (reuse_previous)
{
if (!user)
user = PQuser(o_conn);
if (host && strcmp(host, PQhost(o_conn)) == 0 &&
has_hostaddr(o_conn))
{
hostaddr = PQhostaddr(o_conn);
}
if (!host)
{
host = PQhost(o_conn);
if (has_hostaddr(o_conn))
hostaddr = PQhostaddr(o_conn);
}
if (!port)
port = PQport(o_conn);
}
/*
* Any change in the parameters read above makes us discard the password.
* We also discard it if we're to use a conninfo rather than the
* positional syntax.
*/
if (has_connection_string)
keep_password = false;
cinfo = PQconninfo(o_conn);
else
keep_password =
(user && PQuser(o_conn) && strcmp(user, PQuser(o_conn)) == 0) &&
(host && PQhost(o_conn) && strcmp(host, PQhost(o_conn)) == 0) &&
(port && PQport(o_conn) && strcmp(port, PQport(o_conn)) == 0);
cinfo = PQconndefaults();
/*
* Grab missing dbname from old connection. No password discard if this
* changes: passwords aren't (usually) database-specific.
*/
if (!dbname && reuse_previous)
if (cinfo)
{
initPQExpBuffer(&connstr);
appendPQExpBufferStr(&connstr, "dbname=");
appendConnStrVal(&connstr, PQdb(o_conn));
dbname = connstr.data;
/* has_connection_string=true would be a dead store */
if (has_connection_string)
{
/* Parse the connstring and insert values into cinfo */
PQconninfoOption *replcinfo;
char *errmsg;
replcinfo = PQconninfoParse(dbname, &errmsg);
if (replcinfo)
{
PQconninfoOption *ci;
PQconninfoOption *replci;
for (ci = cinfo, replci = replcinfo;
ci->keyword && replci->keyword;
ci++, replci++)
{
Assert(strcmp(ci->keyword, replci->keyword) == 0);
/* Insert value from connstring if one was provided */
if (replci->val)
{
/*
* We know that both val strings were allocated by
* libpq, so the least messy way to avoid memory leaks
* is to swap them.
*/
char *swap = replci->val;
replci->val = ci->val;
ci->val = swap;
}
}
Assert(ci->keyword == NULL && replci->keyword == NULL);
/* While here, determine how many option slots there are */
nconnopts = ci - cinfo;
PQconninfoFree(replcinfo);
/* We never re-use a password with a conninfo string. */
keep_password = false;
/* Don't let code below try to inject dbname into params. */
dbname = NULL;
}
else
{
/* PQconninfoParse failed */
if (errmsg)
{
pg_log_error("%s", errmsg);
PQfreemem(errmsg);
}
else
pg_log_error("out of memory");
success = false;
}
}
else
{
/*
* If dbname isn't a connection string, then we'll inject it and
* the other parameters into the keyword array below. (We can't
* easily insert them into the cinfo array because of memory
* management issues: PQconninfoFree would misbehave on Windows.)
* However, to avoid dependencies on the order in which parameters
* appear in the array, make a preliminary scan to set
* keep_password and same_host correctly.
*
* While any change in user, host, or port causes us to ignore the
* old connection's password, we don't force that for dbname,
* since passwords aren't database-specific.
*/
PQconninfoOption *ci;
for (ci = cinfo; ci->keyword; ci++)
{
if (user && strcmp(ci->keyword, "user") == 0)
{
if (!(ci->val && strcmp(user, ci->val) == 0))
keep_password = false;
}
else if (host && strcmp(ci->keyword, "host") == 0)
{
if (ci->val && strcmp(host, ci->val) == 0)
same_host = true;
else
keep_password = false;
}
else if (port && strcmp(ci->keyword, "port") == 0)
{
if (!(ci->val && strcmp(port, ci->val) == 0))
keep_password = false;
}
}
/* While here, determine how many option slots there are */
nconnopts = ci - cinfo;
}
}
else
connstr.data = NULL;
{
/* We failed to create the cinfo structure */
pg_log_error("out of memory");
success = false;
}
/*
* If the user asked to be prompted for a password, ask for one now. If
@ -3156,13 +3208,13 @@ do_connect(enum trivalue reuse_previous_specification,
* the postmaster's log. But libpq offers no API that would let us obtain
* a password and then continue with the first connection attempt.
*/
if (pset.getPassword == TRI_YES)
if (pset.getPassword == TRI_YES && success)
{
/*
* If a connstring or URI is provided, we can't be sure we know which
* username will be used, since we haven't parsed that argument yet.
* If a connstring or URI is provided, we don't know which username
* will be used, since we haven't dug that out of the connstring.
* Don't risk issuing a misleading prompt. As in startup.c, it does
* not seem worth working harder, since this getPassword option is
* not seem worth working harder, since this getPassword setting is
* normally only used in noninteractive cases.
*/
password = prompt_for_password(has_connection_string ? NULL : user);
@ -3176,57 +3228,60 @@ do_connect(enum trivalue reuse_previous_specification,
password = NULL;
}
while (true)
/* Loop till we have a connection or fail, which we might've already */
while (success)
{
#define PARAMS_ARRAY_SIZE 9
const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
const char **values = pg_malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
int paramnum = -1;
keywords[++paramnum] = "host";
values[paramnum] = host;
if (hostaddr && *hostaddr)
{
keywords[++paramnum] = "hostaddr";
values[paramnum] = hostaddr;
}
keywords[++paramnum] = "port";
values[paramnum] = port;
keywords[++paramnum] = "user";
values[paramnum] = user;
const char **keywords = pg_malloc((nconnopts + 1) * sizeof(*keywords));
const char **values = pg_malloc((nconnopts + 1) * sizeof(*values));
int paramnum = 0;
PQconninfoOption *ci;
/*
* Position in the array matters when the dbname is a connection
* string, because settings in a connection string override earlier
* array entries only. Thus, user= in the connection string always
* takes effect, but client_encoding= often will not.
* 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.
*
* If you change this code, also change the initial-connection code in
* 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.
*/
keywords[++paramnum] = "dbname";
values[paramnum] = dbname;
keywords[++paramnum] = "password";
values[paramnum] = password;
keywords[++paramnum] = "fallback_application_name";
values[paramnum] = pset.progname;
keywords[++paramnum] = "client_encoding";
values[paramnum] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto";
for (ci = cinfo; ci->keyword; ci++)
{
keywords[paramnum] = ci->keyword;
if (dbname && strcmp(ci->keyword, "dbname") == 0)
values[paramnum++] = dbname;
else if (user && strcmp(ci->keyword, "user") == 0)
values[paramnum++] = user;
else if (host && strcmp(ci->keyword, "host") == 0)
values[paramnum++] = host;
else if (host && !same_host && strcmp(ci->keyword, "hostaddr") == 0)
{
/* If we're changing the host value, drop any old hostaddr */
values[paramnum++] = NULL;
}
else if (port && strcmp(ci->keyword, "port") == 0)
values[paramnum++] = port;
else if (strcmp(ci->keyword, "password") == 0)
values[paramnum++] = password;
else if (strcmp(ci->keyword, "fallback_application_name") == 0)
values[paramnum++] = pset.progname;
else if (strcmp(ci->keyword, "client_encoding") == 0)
values[paramnum++] = (pset.notty || getenv("PGCLIENTENCODING")) ? NULL : "auto";
else if (ci->val)
values[paramnum++] = ci->val;
/* else, don't bother making libpq parse this keyword */
}
/* add array terminator */
keywords[++paramnum] = NULL;
keywords[paramnum] = NULL;
values[paramnum] = NULL;
n_conn = PQconnectdbParams(keywords, values, true);
/* Note we do not want libpq to re-expand the dbname parameter */
n_conn = PQconnectdbParams(keywords, values, false);
pg_free(keywords);
pg_free(values);
/* We can immediately discard the password -- no longer needed */
if (password)
pg_free(password);
if (PQstatus(n_conn) == CONNECTION_OK)
break;
@ -3242,9 +3297,28 @@ do_connect(enum trivalue reuse_previous_specification,
*/
password = prompt_for_password(PQuser(n_conn));
PQfinish(n_conn);
n_conn = NULL;
continue;
}
/*
* We'll report the error below ... unless n_conn is NULL, indicating
* that libpq didn't have enough memory to make a PGconn.
*/
if (n_conn == NULL)
pg_log_error("out of memory");
success = false;
} /* end retry loop */
/* Release locally allocated data, whether we succeeded or not */
if (password)
pg_free(password);
if (cinfo)
PQconninfoFree(cinfo);
if (!success)
{
/*
* Failed to connect to the database. In interactive mode, keep the
* previous connection to the DB; in scripting mode, close our
@ -3252,7 +3326,11 @@ do_connect(enum trivalue reuse_previous_specification,
*/
if (pset.cur_cmd_interactive)
{
pg_log_info("%s", PQerrorMessage(n_conn));
if (n_conn)
{
pg_log_info("%s", PQerrorMessage(n_conn));
PQfinish(n_conn);
}
/* pset.db is left unmodified */
if (o_conn)
@ -3260,7 +3338,12 @@ do_connect(enum trivalue reuse_previous_specification,
}
else
{
pg_log_error("\\connect: %s", PQerrorMessage(n_conn));
if (n_conn)
{
pg_log_error("\\connect: %s", PQerrorMessage(n_conn));
PQfinish(n_conn);
}
if (o_conn)
{
/*
@ -3274,13 +3357,8 @@ do_connect(enum trivalue reuse_previous_specification,
}
}
PQfinish(n_conn);
if (connstr.data)
termPQExpBuffer(&connstr);
return false;
}
if (connstr.data)
termPQExpBuffer(&connstr);
/*
* Replace the old connection with the new one, and update