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

The following bug has been logged online:

Bug reference: 5304
Logged by: Fujii Masao
Email address: masao(dot)fujii(at)gmail(dot)com
PostgreSQL version: HEAD
Operating system: RHEL5.1
Description: psql using conninfo fails in connecting to the server
Details:

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 '='.

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(dot)linnakangas(at)enterprisedb(dot)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(dot)fujii(at)gmail(dot)com>:
> On Mon, Feb 1, 2010 at 5:31 PM, Heikki Linnakangas
> <heikki(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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(dot)linnakangas(at)enterprisedb(dot)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(at)joeconway(dot)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(at)joeconway(dot)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(at)joeconway(dot)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(dot)fujii(at)gmail(dot)com> writes:
> On Wed, Feb 3, 2010 at 10:05 AM, Joe Conway <mail(at)joeconway(dot)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(at)joeconway(dot)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(at)joeconway(dot)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(at)joeconway(dot)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(at)joeconway(dot)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(at)joeconway(dot)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(at)joeconway(dot)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

On 02/02/2010 10:23 PM, Tom Lane wrote:
> Joe Conway <mail(at)joeconway(dot)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.

OK, this one includes pg_dump(all)/pg_restore and common.c from
bin/scripts (createdb, vacuumdb, etc). I still need to adjust the docs,
but other than that any remaining complaints?

Joe

On Thu, Feb 4, 2010 at 1:26 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
> On 02/02/2010 10:23 PM, Tom Lane wrote:
>> Joe Conway <mail(at)joeconway(dot)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.
>
> OK, this one includes pg_dump(all)/pg_restore and common.c from
> bin/scripts (createdb, vacuumdb, etc). I still need to adjust the docs,
> but other than that any remaining complaints?

Thanks a lot!

Here is the small comments:

* expand_dbname is defined as a "bool" value in PQconnectdbParams()
and PQconnectStartParams(). But we should hide such a "bool" from
an user-visible API, and use an "int" instead?

* conninfo_array_parse() calls PQconninfoFree(str_options) as soon
as one "dbname" keyword is found. So if more than one "dbname"
keywords are unexpectedly specified in PQconnectdbParams(), the
str_options would be free()-ed doubly.

Regards,

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

On 02/04/2010 01:23 AM, Fujii Masao wrote:
> On Thu, Feb 4, 2010 at 1:26 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>> OK, this one includes pg_dump(all)/pg_restore and common.c from
>> bin/scripts (createdb, vacuumdb, etc). I still need to adjust the docs,
>> but other than that any remaining complaints?

> * expand_dbname is defined as a "bool" value in PQconnectdbParams()
> and PQconnectStartParams(). But we should hide such a "bool" from
> an user-visible API, and use an "int" instead?

Yes, I suppose there is precedence for that.

> * conninfo_array_parse() calls PQconninfoFree(str_options) as soon
> as one "dbname" keyword is found. So if more than one "dbname"
> keywords are unexpectedly specified in PQconnectdbParams(), the
> str_options would be free()-ed doubly.

Great catch -- thank you!

Thanks for the review. I'll do a documentation update, make these
changes, and commit later today if I don't hear any other objections.

Joe

On 02/04/2010 08:31 AM, Joe Conway wrote:
> On 02/04/2010 01:23 AM, Fujii Masao wrote:
>> On Thu, Feb 4, 2010 at 1:26 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>>> OK, this one includes pg_dump(all)/pg_restore and common.c from
>>> bin/scripts (createdb, vacuumdb, etc). I still need to adjust the docs,
>>> but other than that any remaining complaints?
>
>> * expand_dbname is defined as a "bool" value in PQconnectdbParams()
>> and PQconnectStartParams(). But we should hide such a "bool" from
>> an user-visible API, and use an "int" instead?
>
> Yes, I suppose there is precedence for that.
>
>> * conninfo_array_parse() calls PQconninfoFree(str_options) as soon
>> as one "dbname" keyword is found. So if more than one "dbname"
>> keywords are unexpectedly specified in PQconnectdbParams(), the
>> str_options would be free()-ed doubly.
>
> Great catch -- thank you!
>
> Thanks for the review. I'll do a documentation update, make these
> changes, and commit later today if I don't hear any other objections.

Attached has both items fixed and documentation changes.

Joe

On 02/04/2010 09:37 AM, Joe Conway wrote:
> On 02/04/2010 08:31 AM, Joe Conway wrote:
>> On 02/04/2010 01:23 AM, Fujii Masao wrote:
>>> On Thu, Feb 4, 2010 at 1:26 PM, Joe Conway <mail(at)joeconway(dot)com> wrote:
>>>> OK, this one includes pg_dump(all)/pg_restore and common.c from
>>>> bin/scripts (createdb, vacuumdb, etc). I still need to adjust the docs,
>>>> but other than that any remaining complaints?
>>
>>> * expand_dbname is defined as a "bool" value in PQconnectdbParams()
>>> and PQconnectStartParams(). But we should hide such a "bool" from
>>> an user-visible API, and use an "int" instead?
>>
>> Yes, I suppose there is precedence for that.
>>
>>> * conninfo_array_parse() calls PQconninfoFree(str_options) as soon
>>> as one "dbname" keyword is found. So if more than one "dbname"
>>> keywords are unexpectedly specified in PQconnectdbParams(), the
>>> str_options would be free()-ed doubly.
>>
>> Great catch -- thank you!
>>
>> Thanks for the review. I'll do a documentation update, make these
>> changes, and commit later today if I don't hear any other objections.
>
> Attached has both items fixed and documentation changes.

r4 patch committed

Joe