BUG #5304: psql using conninfo fails in connecting to the server

Fujii Masao wrote:

In HEAD, psql using conninfo fails in connecting to the server as follows.

$ bin/psql "host=localhost"
psql: FATAL: database "host=localhost" does not exist

This is because the recently-introduced PQconnectStartParams()
doesn't handle correctly the dbname parameter containing '='.

Hmm, I don't think that was ever really supposed to work, it was
accidental that it did. I certainly didn't realize you could do that.
The documented way to do that is:

bin/psql --host=localhost

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Mon, Feb 1, 2010 at 5:31 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

In HEAD, psql using conninfo fails in connecting to the server as follows.

  $ bin/psql "host=localhost"
  psql: FATAL:  database "host=localhost" does not exist

This is because the recently-introduced PQconnectStartParams()
doesn't handle correctly the dbname parameter containing '='.

Hmm, I don't think that was ever really supposed to work, it was
accidental that it did. I certainly didn't realize you could do that.
The documented way to do that is:

bin/psql --host=localhost

Really? According to the document, psql using conninfo seems to
be supported.

http://developer.postgresql.org/pgdocs/postgres/app-psql.html#R2-APP-PSQL-CONNECTING

An alternative way to specify connection parameters is in a conninfo
string, which is used instead of a database name. This mechanism give
you very wide control over the connection. For example:

$ psql "service=myservice sslmode=require"

Am I missing something?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

2010/2/1 Fujii Masao <masao.fujii@gmail.com>:

On Mon, Feb 1, 2010 at 5:31 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

In HEAD, psql using conninfo fails in connecting to the server as follows.

  $ bin/psql "host=localhost"
  psql: FATAL:  database "host=localhost" does not exist

This is because the recently-introduced PQconnectStartParams()
doesn't handle correctly the dbname parameter containing '='.

Hmm, I don't think that was ever really supposed to work, it was
accidental that it did. I certainly didn't realize you could do that.
The documented way to do that is:

bin/psql --host=localhost

Really? According to the document, psql using conninfo seems to
be supported.

http://developer.postgresql.org/pgdocs/postgres/app-psql.html#R2-APP-PSQL-CONNECTING

Yes, that is definitely supposed to work. It was intentionally added
in 8.3 and is very useful :-)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Fujii Masao wrote:

In HEAD, psql using conninfo fails in connecting to the server as follows.

$ bin/psql "host=localhost"
psql: FATAL: database "host=localhost" does not exist

This is because the recently-introduced PQconnectStartParams()
doesn't handle correctly the dbname parameter containing '='.

Hmm, I don't think that was ever really supposed to work, it was
accidental that it did.

No, it was intentional.

regards, tom lane

On 02/01/2010 08:06 AM, Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Fujii Masao wrote:

In HEAD, psql using conninfo fails in connecting to the server as follows.

$ bin/psql "host=localhost"
psql: FATAL: database "host=localhost" does not exist

This is because the recently-introduced PQconnectStartParams()
doesn't handle correctly the dbname parameter containing '='.

Hmm, I don't think that was ever really supposed to work, it was
accidental that it did.

No, it was intentional.

Will fix. Give me a day or so though.

Joe

On 02/01/2010 08:06 AM, Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Fujii Masao wrote:

In HEAD, psql using conninfo fails in connecting to the server as follows.

$ bin/psql "host=localhost"
psql: FATAL: database "host=localhost" does not exist

This is because the recently-introduced PQconnectStartParams()
doesn't handle correctly the dbname parameter containing '='.

Hmm, I don't think that was ever really supposed to work, it was
accidental that it did.

No, it was intentional.

Here's a patch.

If "=" is found in the dbname psql argument, the argument is assumed to
be a conninfo string. In that case, append application_name to the
conninfo and use PQsetdbLogin() as before. Otherwise use the new
PQconnectdbParams().

Also only uses static assignments for array constructors.

Objections?

Thanks,

Joe

On Wed, Feb 3, 2010 at 10:05 AM, Joe Conway <mail@joeconway.com> wrote:

Here's a patch.

Thanks!

If "=" is found in the dbname psql argument, the argument is assumed to
be a conninfo string. In that case, append application_name to the
conninfo and use PQsetdbLogin() as before. Otherwise use the new
PQconnectdbParams().

Also only uses static assignments for array constructors.

Objections?

I think that PQconnectdbParams() rather than psql should handle the
dbname containing "=". Otherwise whenever we use PQconnectdbParams(),
we would have to check for the content of the dbname before calling
it in the future application. Which looks very messy for me.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 02/02/2010 05:46 PM, Fujii Masao wrote:

On Wed, Feb 3, 2010 at 10:05 AM, Joe Conway <mail@joeconway.com> wrote:

Objections?

I think that PQconnectdbParams() rather than psql should handle the
dbname containing "=". Otherwise whenever we use PQconnectdbParams(),
we would have to check for the content of the dbname before calling
it in the future application. Which looks very messy for me.

But I thought the whole point of PQconnectdbParams() was to provide an
extensible way to accept parameters when they are already parsed? It
doesn't make any sense to me to have conninfo parsing capability built
into PQconnectdbParams(). For that matter it's kind of an ugly hack that
PQsetdbLogin() supports it, IMHO.

Joe

Joe Conway <mail@joeconway.com> writes:

If "=" is found in the dbname psql argument, the argument is assumed to
be a conninfo string. In that case, append application_name to the
conninfo and use PQsetdbLogin() as before. Otherwise use the new
PQconnectdbParams().

This seems bogus on a couple of levels. First off, I thought the idea
was to get away from using PQsetdbLogin at all. If we go down this path
we'll never be rid of it. Second, to preserve backwards compatibility
we will have to duplicate this same type of logic in any of the other
places we want to replace PQsetdbLogin (because it's actually
PQsetdbLogin that implements the dbname-containing-equal-sign special
case in prior releases). I count nine remaining calls of that function
between bin/ and contrib/, at least some of which were supposed to get
application_name-ified before release.

While I'm looking at this, there's another bogosity in the original
patch: it neglected the PQsetdbLogin call in psql/command.c, meaning
that doing \c would result in losing the application_name setting.

I'm not entirely sure about a better way to do it, but this approach
seems rather unsatisfactory.

Also only uses static assignments for array constructors.

Uh, no, you're still depending on a non-static constructor for the
keywords[] array. I'm not at all certain whether my portability
concern is still valid in 2010, but if it is, this doesn't fix it.
This doesn't do very much to help with the maintenance risk about
keeping the two arrays in step, either --- now there's neither a
direct nor a visual matchup between the entries. I'd suggest
something like

keywords[0] = "host";
values[0] = options.host;
keywords[1] = "port";
values[1] = options.port;
...

regards, tom lane

On 02/02/2010 05:59 PM, Tom Lane wrote:

This seems bogus on a couple of levels. First off, I thought the idea
was to get away from using PQsetdbLogin at all. If we go down this path
we'll never be rid of it. Second, to preserve backwards compatibility
we will have to duplicate this same type of logic in any of the other
places we want to replace PQsetdbLogin (because it's actually
PQsetdbLogin that implements the dbname-containing-equal-sign special
case in prior releases). I count nine remaining calls of that function
between bin/ and contrib/, at least some of which were supposed to get
application_name-ified before release.

While I'm looking at this, there's another bogosity in the original
patch: it neglected the PQsetdbLogin call in psql/command.c, meaning
that doing \c would result in losing the application_name setting.

OK, based on this and the similar complaint fro Fujii-san, I guess I'll
have another go at it. I didn't understand that this patch was leading
to replacement of PQsetdbLogin() entirely -- should I go ahead and
eliminate use of PQsetdbLogin() in our source tree now?

concern is still valid in 2010, but if it is, this doesn't fix it.
This doesn't do very much to help with the maintenance risk about
keeping the two arrays in step, either --- now there's neither a
direct nor a visual matchup between the entries. I'd suggest
something like

keywords[0] = "host";
values[0] = options.host;
keywords[1] = "port";
values[1] = options.port;

Will do.

Joe

Fujii Masao <masao.fujii@gmail.com> writes:

On Wed, Feb 3, 2010 at 10:05 AM, Joe Conway <mail@joeconway.com> wrote:

Objections?

I think that PQconnectdbParams() rather than psql should handle the
dbname containing "=". Otherwise whenever we use PQconnectdbParams(),
we would have to check for the content of the dbname before calling
it in the future application. Which looks very messy for me.

Yeah, I just complained about the same thing. However I don't think
we should make PQconnectdbParams do that unconditionally. In a lot of
applications, it is a key advantage of PQconnectdbParams that there's
no possibility of funny characters in the arguments resulting in "SQL
injection", ie, somebody being able to set connection parameters they
weren't supposed to. Even without any malicious intent, having to
think about quoting and so forth destroys a lot of the value.

Since we haven't yet released PQconnectdbParams, it's not too late
to twiddle its API. What I'm thinking about is an additional
boolean parameter "expand_dbname", which only if true would enable
treating an equal-sign-containing dbname like a conninfo string.
Passing true would be okay for command-line apps where the user is
supposed to control all the conn parameters anyway, but apps that
want more security would pass false.

We should also give more than zero thought to how values coming from the
expanded dbname should interact with values from other arguments to
PQconnectdbParams --- which should override which? And should there be
an order dependency?

regards, tom lane

On 02/02/2010 06:10 PM, Tom Lane wrote:

Since we haven't yet released PQconnectdbParams, it's not too late
to twiddle its API. What I'm thinking about is an additional
boolean parameter "expand_dbname", which only if true would enable
treating an equal-sign-containing dbname like a conninfo string.
Passing true would be okay for command-line apps where the user is
supposed to control all the conn parameters anyway, but apps that
want more security would pass false.

OK

We should also give more than zero thought to how values coming from the
expanded dbname should interact with values from other arguments to
PQconnectdbParams --- which should override which? And should there be
an order dependency?

My first thought was to duplicate the logic used by PQsetdbLogin(). It
uses the conninfo string, fills in the defaults using connectOptions1(),
applies the supplied other arguments overriding the defaults, and then
finally computes derived options with connectOptions2(). It is
essentially the same as PQconnectdb() except the supplied parameters are
used before setting the derived options.

Joe

Joe Conway <mail@joeconway.com> writes:

On 02/02/2010 06:10 PM, Tom Lane wrote:

We should also give more than zero thought to how values coming from the
expanded dbname should interact with values from other arguments to
PQconnectdbParams --- which should override which? And should there be
an order dependency?

My first thought was to duplicate the logic used by PQsetdbLogin(). It
uses the conninfo string, fills in the defaults using connectOptions1(),
applies the supplied other arguments overriding the defaults, and then
finally computes derived options with connectOptions2(). It is
essentially the same as PQconnectdb() except the supplied parameters are
used before setting the derived options.

The difference with PQconnectdbParams is that the dbname might not be
the first thing in the parameter array. I think that a straightforward
implementation would have the effect of the expanded dbname overriding
parameters given before it, but not those given after it. Consider

keyword[0] = "port";
values[0] = "5678";
keyword[1] = "dbname";
values[1] = "dbname = db user = foo port = 9999";
keyword[2] = "user";
values[2] = "uu";

What I'm imagining is that this would end up equivalent to
dbname = db user = uu port = 9999. That's probably reasonable,
and maybe even useful, as long as it's documented.

regards, tom lane

On 02/02/2010 06:40 PM, Tom Lane wrote:

The difference with PQconnectdbParams is that the dbname might not be
the first thing in the parameter array. I think that a straightforward
implementation would have the effect of the expanded dbname overriding
parameters given before it, but not those given after it. Consider

keyword[0] = "port";
values[0] = "5678";
keyword[1] = "dbname";
values[1] = "dbname = db user = foo port = 9999";
keyword[2] = "user";
values[2] = "uu";

What I'm imagining is that this would end up equivalent to
dbname = db user = uu port = 9999. That's probably reasonable,
and maybe even useful, as long as it's documented.

No doc changes yet, and I still have not corrected the earlier mentioned
issue,

While I'm looking at this, there's another bogosity in the original
patch: it neglected the PQsetdbLogin call in psql/command.c, meaning
that doing \c would result in losing the application_name setting.

but I wanted to get feedback before going further.

This patch implements Tom's idea above. Note that I also rearranged the
parameters for the call from psql so that dbname is last, therefore
allowing a conninfo to override all other settings. The result looks like:

keywords[0] = "host";
values[0] = options.host;
keywords[1] = "port";
values[1] = options.port;
keywords[2] = "user";
values[2] = options.username;
keywords[3] = "password";
values[3] = password;
keywords[4] = "application_name";
values[4] = pset.progname;
keywords[5] = "dbname";
values[5] = (options.action == ACT_LIST_DB &&
options.dbname == NULL) ?
"postgres" : options.dbname;
keywords[6] = NULL;
values[6] = NULL;

Which produces:

# psql -U postgres "dbname=regression user=fred application_name='joe\'s
app'"
psql (8.5devel)
Type "help" for help.

regression=> select backend_start, application_name from pg_stat_activity ;
backend_start | application_name
-------------------------------+------------------
2010-02-02 21:44:55.969202-08 | joe's app
(1 row)

regression=> select current_user;
current_user
--------------
fred
(1 row)

Are there any of the psql parameters that we do not want to allow to be
overridden by the conninfo string?

Joe

Joe Conway <mail@joeconway.com> writes:

Are there any of the psql parameters that we do not want to allow to be
overridden by the conninfo string?

Actually, now that I think about it, psql shouldn't be setting
application_name at all. It should be setting
fallback_application_name, and I think that should be after the dbname
so that it's not overridable. The way it's being done now destroys the
usefulness of the PGAPPNAME environment variable.

regards, tom lane

On 02/02/2010 10:08 PM, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

Are there any of the psql parameters that we do not want to allow to be
overridden by the conninfo string?

Actually, now that I think about it, psql shouldn't be setting
application_name at all. It should be setting
fallback_application_name, and I think that should be after the dbname
so that it's not overridable. The way it's being done now destroys the
usefulness of the PGAPPNAME environment variable.

Easily done. I'll fix psql/command.c and the documentation tomorrow and
post another patch for review before committing.

Should I also be looking to replace all (or most) other instances of
PQsetdbLogin()?

Thanks!

Joe

Joe Conway <mail@joeconway.com> writes:

Should I also be looking to replace all (or most) other instances of
PQsetdbLogin()?

I think we at least wanted to fix pg_dump(all)/pg_restore. Not sure if
the others are worth troubling over.

regards, tom lane

[moving to hackers]

On 02/02/2010 10:23 PM, Tom Lane wrote:

Joe Conway <mail@joeconway.com> writes:

Should I also be looking to replace all (or most) other instances of
PQsetdbLogin()?

I think we at least wanted to fix pg_dump(all)/pg_restore. Not sure if
the others are worth troubling over.

Any objection if I add these two macros to libpq-fe.h?
-------------------
+ /* Useful macros for PQconnectdbParams() and PQconnectStartParams() */
+ #define DECL_PARAMS_ARRAYS(PARAMS_ARRAY_SIZE) \
+       const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * \
+                                         sizeof(*keywords)); \
+       const char **values = pg_malloc(PARAMS_ARRAY_SIZE * \
+                                       sizeof(*values))
+ #define SET_PARAMS_ARRAYS(__kv_idx__,__kv_kw__,__kv_v__) \
+       keywords[__kv_idx__]    = __kv_kw__; \
+       values[__kv_idx__]      = __kv_v__

That way this:
-------------------
! #define PARAMS_ARRAY_SIZE 7
! const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE *
! sizeof(*keywords));
! const char **values = pg_malloc(PARAMS_ARRAY_SIZE *
! sizeof(*values));
!
! keywords[0] = "host";
! values[0] = options.host;
! keywords[1] = "port";
! values[1] = options.port;
! keywords[2] = "user";
! values[2] = options.username;
! keywords[3] = "password";
! values[3] = password;
! keywords[4] = "dbname";
! values[4] = (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ?
! "postgres" : options.dbname;
! keywords[5] = "fallback_application_name";
! values[5] = pset.progname;
! keywords[6] = NULL;
! values[6] = NULL;

becomes this:
-------------------
! DECL_PARAMS_ARRAYS(7);

! SET_PARAMS_ARRAYS(0,"host",options.host);
! SET_PARAMS_ARRAYS(1,"port",options.port);
! SET_PARAMS_ARRAYS(2,"user",options.username);
! SET_PARAMS_ARRAYS(3,"password",password);
! SET_PARAMS_ARRAYS(4,"dbname",(options.action == ACT_LIST_DB &&
! options.dbname == NULL) ?
! "postgres" : options.dbname);
! SET_PARAMS_ARRAYS(5,"fallback_application_name",pset.progname);
! SET_PARAMS_ARRAYS(6,NULL,NULL);

I suppose if we do this maybe we should also do:
#define PARAMS_KEYWORDS keywords
#define PARAMS_VALUES values

so the subsequent use looks like:

pset.db = PQconnectdbParams(PARAMS_KEYWORDS, PARAMS_VALUES, true);

To me it seems easier to read and less error prone. Comments?

Joe

Joe Conway <mail@joeconway.com> writes:

Any objection if I add these two macros to libpq-fe.h?
-------------------
+ /* Useful macros for PQconnectdbParams() and PQconnectStartParams() */
+ #define DECL_PARAMS_ARRAYS(PARAMS_ARRAY_SIZE) \
+       const char **keywords = pg_malloc(PARAMS_ARRAY_SIZE * \
+                                         sizeof(*keywords)); \
+       const char **values = pg_malloc(PARAMS_ARRAY_SIZE * \
+                                       sizeof(*values))
+ #define SET_PARAMS_ARRAYS(__kv_idx__,__kv_kw__,__kv_v__) \
+       keywords[__kv_idx__]    = __kv_kw__; \
+       values[__kv_idx__]      = __kv_v__

Yes. Exposing pg_malloc is 100% wrong, and would be even if you'd
remembered the subsequent free() ;-)

It does not seem especially useful either. In most cases the array size
is constant and there's no reason to not just make it a local in the
calling function.

regards, tom lane