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.fujii@gmail.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 existThis 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 existThis 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 existThis 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 existThis 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 existThis 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 existThis 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
Attachments:
psql-PQconnectParams-fixes.r0.patchtext/x-patch; name=psql-PQconnectParams-fixes.r0.patchDownload
Index: src/bin/psql/startup.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.159
diff -c -r1.159 startup.c
*** src/bin/psql/startup.c 28 Jan 2010 06:28:26 -0000 1.159
--- src/bin/psql/startup.c 3 Feb 2010 00:53:08 -0000
***************
*** 90,97 ****
char *password = NULL;
char *password_prompt = NULL;
bool new_pass;
- const char *keywords[] = {"host","port","dbname","user",
- "password","application_name",NULL};
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("psql"));
--- 90,95 ----
***************
*** 173,192 ****
/* loop until we have a password if requested by backend */
do
{
! const char *values[] = {
! options.host,
! options.port,
! (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ? "postgres" : options.dbname,
! options.username,
! password,
! pset.progname,
! NULL
! };
!
! new_pass = false;
! pset.db = PQconnectdbParams(keywords, values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
--- 171,218 ----
/* loop until we have a password if requested by backend */
do
{
! /*
! * If the dbName parameter contains '=', assume it's a conninfo string.
! */
! if (options.dbname && strchr(options.dbname, '='))
! {
! PQExpBuffer dbName = createPQExpBuffer();
!
! appendPQExpBuffer(dbName, "%s application_name=%s",
! options.dbname, pset.progname);
! new_pass = false;
! pset.db = PQsetdbLogin(options.host, options.port, NULL, NULL,
! dbName->data, options.username, password);
! destroyPQExpBuffer(dbName);
! }
! else
! {
! const char *keywords[] = {
! "host",
! "port",
! "dbname",
! "user",
! "password",
! "application_name",
! NULL
! };
! const char **values = pg_malloc(sizeof(keywords));
!
! values[0] = options.host;
! values[1] = options.port;
! values[2] = (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ?
! "postgres" : options.dbname;
! values[3] = options.username;
! values[4] = password;
! values[5] = pset.progname;
! values[6] = NULL;
!
! new_pass = false;
! pset.db = PQconnectdbParams(keywords, values);
! free(values);
! }
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
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 likekeywords[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. Considerkeyword[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
Attachments:
psql-PQconnectParams-fixes.r1.patchtext/x-patch; name=psql-PQconnectParams-fixes.r1.patchDownload
Index: src/bin/psql/startup.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.159
diff -c -r1.159 startup.c
*** src/bin/psql/startup.c 28 Jan 2010 06:28:26 -0000 1.159
--- src/bin/psql/startup.c 3 Feb 2010 05:41:54 -0000
***************
*** 90,97 ****
char *password = NULL;
char *password_prompt = NULL;
bool new_pass;
- const char *keywords[] = {"host","port","dbname","user",
- "password","application_name",NULL};
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("psql"));
--- 90,95 ----
***************
*** 173,192 ****
/* loop until we have a password if requested by backend */
do
{
! const char *values[] = {
! options.host,
! options.port,
! (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ? "postgres" : options.dbname,
! options.username,
! password,
! pset.progname,
! NULL
! };
!
! new_pass = false;
!
! pset.db = PQconnectdbParams(keywords, values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
--- 171,201 ----
/* loop until we have a password if requested by backend */
do
{
! #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] = "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;
!
! new_pass = false;
! pset.db = PQconnectdbParams(keywords, values, true /* expand conninfo string in dbname if found */);
! free(keywords);
! free(values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.385
diff -c -r1.385 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 28 Jan 2010 06:28:26 -0000 1.385
--- src/interfaces/libpq/fe-connect.c 3 Feb 2010 05:29:54 -0000
***************
*** 269,275 ****
PQExpBuffer errorMessage, bool use_defaults);
static PQconninfoOption *conninfo_array_parse(const char **keywords,
const char **values, PQExpBuffer errorMessage,
! bool use_defaults);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
--- 269,275 ----
PQExpBuffer errorMessage, bool use_defaults);
static PQconninfoOption *conninfo_array_parse(const char **keywords,
const char **values, PQExpBuffer errorMessage,
! bool use_defaults, bool expand_dbname);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
***************
*** 336,344 ****
* call succeeded.
*/
PGconn *
! PQconnectdbParams(const char **keywords, const char **values)
{
! PGconn *conn = PQconnectStartParams(keywords, values);
if (conn && conn->status != CONNECTION_BAD)
(void) connectDBComplete(conn);
--- 336,346 ----
* call succeeded.
*/
PGconn *
! PQconnectdbParams(const char **keywords,
! const char **values,
! bool expand_dbname)
{
! PGconn *conn = PQconnectStartParams(keywords, values, expand_dbname);
if (conn && conn->status != CONNECTION_BAD)
(void) connectDBComplete(conn);
***************
*** 400,406 ****
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords, const char **values)
{
PGconn *conn;
PQconninfoOption *connOptions;
--- 402,410 ----
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords,
! const char **values,
! bool expand_dbname)
{
PGconn *conn;
PQconninfoOption *connOptions;
***************
*** 416,422 ****
* Parse the conninfo arrays
*/
connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage, true);
if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
--- 420,427 ----
* Parse the conninfo arrays
*/
connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage,
! true, expand_dbname);
if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
***************
*** 3729,3744 ****
* left in errorMessage.
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
*/
static PQconninfoOption *
conninfo_array_parse(const char **keywords, const char **values,
! PQExpBuffer errorMessage, bool use_defaults)
{
char *tmp;
PQconninfoOption *options;
PQconninfoOption *option;
int i = 0;
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
if (options == NULL)
--- 3734,3786 ----
* left in errorMessage.
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
+ *
+ * If expand_dbname is TRUE, and the value passed for keyword "dbname"
+ * contains an "=", assume it is a conninfo string and process it,
+ * overriding any previously processed conflicting keywords. Subsequent
+ * keywords will take precedence, however.
*/
static PQconninfoOption *
conninfo_array_parse(const char **keywords, const char **values,
! PQExpBuffer errorMessage, bool use_defaults,
! bool expand_dbname)
{
char *tmp;
PQconninfoOption *options;
+ PQconninfoOption *str_options = NULL;
PQconninfoOption *option;
int i = 0;
+ /*
+ * If expand_dbname is true, check keyword "dbname"
+ * to see if val is actually a conninfo string
+ */
+ while(expand_dbname && keywords[i])
+ {
+ const char *pname = keywords[i];
+ const char *pvalue = values[i];
+
+ /* first find "dbname" if any */
+ if (strcmp(pname, "dbname") == 0)
+ {
+ /* next look for "=" in the value */
+ if (pvalue && strchr(pvalue, '='))
+ {
+ /*
+ * Must be a conninfo string, so parse it, but do not
+ * use defaults here -- those get picked up later.
+ * We only want to override for those parameters actually
+ * passed.
+ */
+ str_options = conninfo_parse(pvalue, errorMessage, false);
+ if (str_options == NULL)
+ return NULL;
+ }
+ break;
+ }
+ ++i;
+ }
+
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
if (options == NULL)
***************
*** 3749,3754 ****
--- 3791,3797 ----
}
memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+ i = 0;
/* Parse the keywords/values arrays */
while(keywords[i])
{
***************
*** 3774,3792 ****
return NULL;
}
! /*
! * Store the value
! */
! if (option->val)
! free(option->val);
! option->val = strdup(pvalue);
! if (!option->val)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! PQconninfoFree(options);
! return NULL;
! }
}
++i;
}
--- 3817,3867 ----
return NULL;
}
! /*
! * If we are on the dbname parameter, and we have a parsed
! * conninfo string, copy those parameters across, overriding
! * any existing previous settings
! */
! if (strcmp(pname, "dbname") == 0 && str_options)
! {
! PQconninfoOption *str_option;
!
! for (str_option = str_options; str_option->keyword != NULL; str_option++)
! {
! if (str_option->val != NULL)
! {
! int k;
!
! for (k = 0; options[k].keyword; k++)
! {
! if (strcmp(options[k].keyword, str_option->keyword) == 0)
! {
! if (options[k].val)
! free(options[k].val);
! options[k].val = strdup(str_option->val);
! break;
! }
! }
! }
! }
! PQconninfoFree(str_options);
! }
! else
! {
! /*
! * Store the value, overriding previous settings
! */
! if (option->val)
! free(option->val);
! option->val = strdup(pvalue);
! if (!option->val)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! PQconninfoFree(options);
! return NULL;
! }
! }
}
++i;
}
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.150
diff -c -r1.150 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 28 Jan 2010 06:28:26 -0000 1.150
--- src/interfaces/libpq/libpq-fe.h 3 Feb 2010 02:37:50 -0000
***************
*** 226,237 ****
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
! extern PGconn *PQconnectStartParams(const char **keywords, const char **values);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
! extern PGconn *PQconnectdbParams(const char **keywords, const char **values);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
--- 226,239 ----
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
! extern PGconn *PQconnectStartParams(const char **keywords,
! const char **values, bool expand_dbname);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
! extern PGconn *PQconnectdbParams(const char **keywords,
! const char **values, bool expand_dbname);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
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
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.
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
Attachments:
psql-PQconnectParams-fixes.r3.patchtext/x-patch; name=psql-PQconnectParams-fixes.r3.patchDownload
Index: src/bin/pg_dump/pg_backup_db.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/pg_dump/pg_backup_db.c,v
retrieving revision 1.85
diff -c -r1.85 pg_backup_db.c
*** src/bin/pg_dump/pg_backup_db.c 14 Dec 2009 00:39:11 -0000 1.85
--- src/bin/pg_dump/pg_backup_db.c 4 Feb 2010 03:57:42 -0000
***************
*** 154,163 ****
do
{
new_pass = false;
! newConn = PQsetdbLogin(PQhost(AH->connection), PQport(AH->connection),
! NULL, NULL, newdb,
! newuser, password);
if (!newConn)
die_horribly(AH, modulename, "failed to reconnect to database\n");
--- 154,187 ----
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ die_horribly(AH, modulename, "out of memory\n");
+
+ keywords[0] = "host";
+ values[0] = PQhost(AH->connection);
+ keywords[1] = "port";
+ values[1] = PQport(AH->connection);
+ keywords[2] = "user";
+ values[2] = newuser;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = newdb;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! newConn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
!
if (!newConn)
die_horribly(AH, modulename, "failed to reconnect to database\n");
***************
*** 237,245 ****
*/
do
{
new_pass = false;
! AH->connection = PQsetdbLogin(pghost, pgport, NULL, NULL,
! dbname, username, password);
if (!AH->connection)
die_horribly(AH, modulename, "failed to connect to database\n");
--- 261,293 ----
*/
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ die_horribly(AH, modulename, "out of memory\n");
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = username;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! AH->connection = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
if (!AH->connection)
die_horribly(AH, modulename, "failed to connect to database\n");
***************
*** 697,699 ****
--- 745,748 ----
else
return false;
}
+
Index: src/bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.131
diff -c -r1.131 pg_dumpall.c
*** src/bin/pg_dump/pg_dumpall.c 6 Jan 2010 03:34:41 -0000 1.131
--- src/bin/pg_dump/pg_dumpall.c 4 Feb 2010 03:55:45 -0000
***************
*** 1618,1625 ****
*/
do
{
new_pass = false;
! conn = PQsetdbLogin(pghost, pgport, NULL, NULL, dbname, pguser, password);
if (!conn)
{
--- 1618,1653 ----
*/
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ {
+ fprintf(stderr, _("%s: out of memory\n"), progname);
+ exit(1);
+ }
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = pguser;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! conn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
if (!conn)
{
Index: src/bin/psql/command.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/command.c,v
retrieving revision 1.213
diff -c -r1.213 command.c
*** src/bin/psql/command.c 2 Jan 2010 16:57:59 -0000 1.213
--- src/bin/psql/command.c 4 Feb 2010 01:18:34 -0000
***************
*** 1213,1219 ****
* Connects to a database with given parameters. If there exists an
* established connection, NULL values will be replaced with the ones
* in the current connection. Otherwise NULL will be passed for that
! * parameter to PQsetdbLogin(), so the libpq defaults will be used.
*
* In interactive mode, if connection fails with the given parameters,
* the old connection will be kept.
--- 1213,1219 ----
* Connects to a database with given parameters. If there exists an
* established connection, NULL values will be replaced with the ones
* in the current connection. Otherwise NULL will be passed for that
! * parameter to PQconnectdbParams(), so the libpq defaults will be used.
*
* In interactive mode, if connection fails with the given parameters,
* the old connection will be kept.
***************
*** 1255,1262 ****
while (true)
{
! n_conn = PQsetdbLogin(host, port, NULL, NULL,
! dbname, user, password);
/* We can immediately discard the password -- no longer needed */
if (password)
--- 1255,1283 ----
while (true)
{
! #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] = host;
! keywords[1] = "port";
! values[1] = port;
! keywords[2] = "user";
! values[2] = user;
! keywords[3] = "password";
! values[3] = password;
! keywords[4] = "dbname";
! values[4] = dbname;
! keywords[5] = "fallback_application_name";
! values[5] = pset.progname;
! keywords[6] = NULL;
! values[6] = NULL;
!
! n_conn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
/* We can immediately discard the password -- no longer needed */
if (password)
Index: src/bin/psql/startup.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.159
diff -c -r1.159 startup.c
*** src/bin/psql/startup.c 28 Jan 2010 06:28:26 -0000 1.159
--- src/bin/psql/startup.c 4 Feb 2010 02:31:32 -0000
***************
*** 90,97 ****
char *password = NULL;
char *password_prompt = NULL;
bool new_pass;
- const char *keywords[] = {"host","port","dbname","user",
- "password","application_name",NULL};
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("psql"));
--- 90,95 ----
***************
*** 173,192 ****
/* loop until we have a password if requested by backend */
do
{
! const char *values[] = {
! options.host,
! options.port,
! (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ? "postgres" : options.dbname,
! options.username,
! password,
! pset.progname,
! NULL
! };
!
! new_pass = false;
!
! pset.db = PQconnectdbParams(keywords, values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
--- 171,201 ----
/* loop until we have a password if requested by backend */
do
{
! #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;
!
! new_pass = false;
! pset.db = PQconnectdbParams(keywords, values, true);
! free(keywords);
! free(values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
Index: src/bin/scripts/common.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/scripts/common.c,v
retrieving revision 1.38
diff -c -r1.38 common.c
*** src/bin/scripts/common.c 2 Jan 2010 16:58:00 -0000 1.38
--- src/bin/scripts/common.c 4 Feb 2010 03:54:14 -0000
***************
*** 108,115 ****
*/
do
{
new_pass = false;
! conn = PQsetdbLogin(pghost, pgport, NULL, NULL, dbname, pguser, password);
if (!conn)
{
--- 108,143 ----
*/
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ {
+ fprintf(stderr, _("%s: out of memory\n"), progname);
+ exit(1);
+ }
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = pguser;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! conn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
if (!conn)
{
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.385
diff -c -r1.385 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 28 Jan 2010 06:28:26 -0000 1.385
--- src/interfaces/libpq/fe-connect.c 3 Feb 2010 16:56:45 -0000
***************
*** 269,275 ****
PQExpBuffer errorMessage, bool use_defaults);
static PQconninfoOption *conninfo_array_parse(const char **keywords,
const char **values, PQExpBuffer errorMessage,
! bool use_defaults);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
--- 269,275 ----
PQExpBuffer errorMessage, bool use_defaults);
static PQconninfoOption *conninfo_array_parse(const char **keywords,
const char **values, PQExpBuffer errorMessage,
! bool use_defaults, bool expand_dbname);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
***************
*** 336,344 ****
* call succeeded.
*/
PGconn *
! PQconnectdbParams(const char **keywords, const char **values)
{
! PGconn *conn = PQconnectStartParams(keywords, values);
if (conn && conn->status != CONNECTION_BAD)
(void) connectDBComplete(conn);
--- 336,346 ----
* call succeeded.
*/
PGconn *
! PQconnectdbParams(const char **keywords,
! const char **values,
! bool expand_dbname)
{
! PGconn *conn = PQconnectStartParams(keywords, values, expand_dbname);
if (conn && conn->status != CONNECTION_BAD)
(void) connectDBComplete(conn);
***************
*** 400,406 ****
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords, const char **values)
{
PGconn *conn;
PQconninfoOption *connOptions;
--- 402,410 ----
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords,
! const char **values,
! bool expand_dbname)
{
PGconn *conn;
PQconninfoOption *connOptions;
***************
*** 416,422 ****
* Parse the conninfo arrays
*/
connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage, true);
if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
--- 420,427 ----
* Parse the conninfo arrays
*/
connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage,
! true, expand_dbname);
if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
***************
*** 3729,3744 ****
* left in errorMessage.
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
*/
static PQconninfoOption *
conninfo_array_parse(const char **keywords, const char **values,
! PQExpBuffer errorMessage, bool use_defaults)
{
char *tmp;
PQconninfoOption *options;
PQconninfoOption *option;
int i = 0;
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
if (options == NULL)
--- 3734,3786 ----
* left in errorMessage.
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
+ *
+ * If expand_dbname is TRUE, and the value passed for keyword "dbname"
+ * contains an "=", assume it is a conninfo string and process it,
+ * overriding any previously processed conflicting keywords. Subsequent
+ * keywords will take precedence, however.
*/
static PQconninfoOption *
conninfo_array_parse(const char **keywords, const char **values,
! PQExpBuffer errorMessage, bool use_defaults,
! bool expand_dbname)
{
char *tmp;
PQconninfoOption *options;
+ PQconninfoOption *str_options = NULL;
PQconninfoOption *option;
int i = 0;
+ /*
+ * If expand_dbname is true, check keyword "dbname"
+ * to see if val is actually a conninfo string
+ */
+ while(expand_dbname && keywords[i])
+ {
+ const char *pname = keywords[i];
+ const char *pvalue = values[i];
+
+ /* first find "dbname" if any */
+ if (strcmp(pname, "dbname") == 0)
+ {
+ /* next look for "=" in the value */
+ if (pvalue && strchr(pvalue, '='))
+ {
+ /*
+ * Must be a conninfo string, so parse it, but do not
+ * use defaults here -- those get picked up later.
+ * We only want to override for those parameters actually
+ * passed.
+ */
+ str_options = conninfo_parse(pvalue, errorMessage, false);
+ if (str_options == NULL)
+ return NULL;
+ }
+ break;
+ }
+ ++i;
+ }
+
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
if (options == NULL)
***************
*** 3749,3754 ****
--- 3791,3797 ----
}
memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+ i = 0;
/* Parse the keywords/values arrays */
while(keywords[i])
{
***************
*** 3774,3792 ****
return NULL;
}
! /*
! * Store the value
! */
! if (option->val)
! free(option->val);
! option->val = strdup(pvalue);
! if (!option->val)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! PQconninfoFree(options);
! return NULL;
! }
}
++i;
}
--- 3817,3867 ----
return NULL;
}
! /*
! * If we are on the dbname parameter, and we have a parsed
! * conninfo string, copy those parameters across, overriding
! * any existing previous settings
! */
! if (strcmp(pname, "dbname") == 0 && str_options)
! {
! PQconninfoOption *str_option;
!
! for (str_option = str_options; str_option->keyword != NULL; str_option++)
! {
! if (str_option->val != NULL)
! {
! int k;
!
! for (k = 0; options[k].keyword; k++)
! {
! if (strcmp(options[k].keyword, str_option->keyword) == 0)
! {
! if (options[k].val)
! free(options[k].val);
! options[k].val = strdup(str_option->val);
! break;
! }
! }
! }
! }
! PQconninfoFree(str_options);
! }
! else
! {
! /*
! * Store the value, overriding previous settings
! */
! if (option->val)
! free(option->val);
! option->val = strdup(pvalue);
! if (!option->val)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! PQconninfoFree(options);
! return NULL;
! }
! }
}
++i;
}
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.150
diff -c -r1.150 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 28 Jan 2010 06:28:26 -0000 1.150
--- src/interfaces/libpq/libpq-fe.h 4 Feb 2010 01:20:00 -0000
***************
*** 226,237 ****
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
! extern PGconn *PQconnectStartParams(const char **keywords, const char **values);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
! extern PGconn *PQconnectdbParams(const char **keywords, const char **values);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
--- 226,239 ----
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
! extern PGconn *PQconnectStartParams(const char **keywords,
! const char **values, bool expand_dbname);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
! extern PGconn *PQconnectdbParams(const char **keywords,
! const char **values, bool expand_dbname);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
On Thu, Feb 4, 2010 at 1:26 PM, Joe Conway <mail@joeconway.com> wrote:
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.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@joeconway.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@joeconway.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
Attachments:
psql-PQconnectParams-fixes.r4.patchtext/x-patch; name=psql-PQconnectParams-fixes.r4.patchDownload
Index: doc/src/sgml/libpq.sgml
===================================================================
RCS file: /opt/src/cvs/pgsql/doc/src/sgml/libpq.sgml,v
retrieving revision 1.296
diff -c -r1.296 libpq.sgml
*** doc/src/sgml/libpq.sgml 28 Jan 2010 06:28:26 -0000 1.296
--- doc/src/sgml/libpq.sgml 4 Feb 2010 17:28:55 -0000
***************
*** 98,104 ****
Makes a new connection to the database server.
<synopsis>
! PGconn *PQconnectdbParams(const char **keywords, const char **values);
</synopsis>
</para>
--- 98,104 ----
Makes a new connection to the database server.
<synopsis>
! PGconn *PQconnectdbParams(const char **keywords, const char **values, int expand_dbname);
</synopsis>
</para>
***************
*** 115,120 ****
--- 115,126 ----
</para>
<para>
+ When <literal>expand_dbname</literal> is non-zero, the
+ <parameter>dbname</parameter> key word value is allowed to be recognized
+ as a <parameter>conninfo</parameter> string. See below for details.
+ </para>
+
+ <para>
The passed arrays can be empty to use all default parameters, or can
contain one or more parameter settings. They should be matched in length.
Processing will stop with the last non-<symbol>NULL</symbol> element
***************
*** 473,478 ****
--- 479,502 ----
is checked. If the environment variable is not set either,
then the indicated built-in defaults are used.
</para>
+
+ <para>
+ If <literal>expand_dbname</literal> is non-zero and
+ <parameter>dbname</parameter> contains an <symbol>=</symbol> sign, it
+ is taken as a <parameter>conninfo</parameter> string in exactly the same way as
+ if it had been passed to <function>PQconnectdb</function>(see below). Previously
+ processed key words will be overridden by key words in the
+ <parameter>conninfo</parameter> string.
+ </para>
+
+ <para>
+ In general key words are processed from the beginning of these arrays in index
+ order. The effect of this is that when key words are repeated, the last processed
+ value is retained. Therefore, through careful placement of the
+ <parameter>dbname</parameter> key word, it is possible to determine what may
+ be overridden by a <parameter>conninfo</parameter> string, and what may not.
+ </para>
+
</listitem>
</varlistentry>
***************
*** 573,579 ****
Make a connection to the database server in a nonblocking manner.
<synopsis>
! PGconn *PQconnectStartParams(const char **keywords, const char **values);
</synopsis>
<synopsis>
--- 597,603 ----
Make a connection to the database server in a nonblocking manner.
<synopsis>
! PGconn *PQconnectStartParams(const char **keywords, const char **values, int expand_dbname);
</synopsis>
<synopsis>
***************
*** 597,604 ****
<para>
With <function>PQconnectStartParams</function>, the database connection is made
using the parameters taken from the <literal>keywords</literal> and
! <literal>values</literal> arrays, as described above for
! <function>PQconnectdbParams</function>.
</para>
<para>
--- 621,628 ----
<para>
With <function>PQconnectStartParams</function>, the database connection is made
using the parameters taken from the <literal>keywords</literal> and
! <literal>values</literal> arrays, and controlled by <literal>expand_dbname</literal>,
! as described above for <function>PQconnectdbParams</function>.
</para>
<para>
Index: src/bin/pg_dump/pg_backup_db.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/pg_dump/pg_backup_db.c,v
retrieving revision 1.85
diff -c -r1.85 pg_backup_db.c
*** src/bin/pg_dump/pg_backup_db.c 14 Dec 2009 00:39:11 -0000 1.85
--- src/bin/pg_dump/pg_backup_db.c 4 Feb 2010 03:57:42 -0000
***************
*** 154,163 ****
do
{
new_pass = false;
! newConn = PQsetdbLogin(PQhost(AH->connection), PQport(AH->connection),
! NULL, NULL, newdb,
! newuser, password);
if (!newConn)
die_horribly(AH, modulename, "failed to reconnect to database\n");
--- 154,187 ----
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ die_horribly(AH, modulename, "out of memory\n");
+
+ keywords[0] = "host";
+ values[0] = PQhost(AH->connection);
+ keywords[1] = "port";
+ values[1] = PQport(AH->connection);
+ keywords[2] = "user";
+ values[2] = newuser;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = newdb;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! newConn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
!
if (!newConn)
die_horribly(AH, modulename, "failed to reconnect to database\n");
***************
*** 237,245 ****
*/
do
{
new_pass = false;
! AH->connection = PQsetdbLogin(pghost, pgport, NULL, NULL,
! dbname, username, password);
if (!AH->connection)
die_horribly(AH, modulename, "failed to connect to database\n");
--- 261,293 ----
*/
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ die_horribly(AH, modulename, "out of memory\n");
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = username;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! AH->connection = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
if (!AH->connection)
die_horribly(AH, modulename, "failed to connect to database\n");
***************
*** 697,699 ****
--- 745,748 ----
else
return false;
}
+
Index: src/bin/pg_dump/pg_dumpall.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.131
diff -c -r1.131 pg_dumpall.c
*** src/bin/pg_dump/pg_dumpall.c 6 Jan 2010 03:34:41 -0000 1.131
--- src/bin/pg_dump/pg_dumpall.c 4 Feb 2010 03:55:45 -0000
***************
*** 1618,1625 ****
*/
do
{
new_pass = false;
! conn = PQsetdbLogin(pghost, pgport, NULL, NULL, dbname, pguser, password);
if (!conn)
{
--- 1618,1653 ----
*/
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ {
+ fprintf(stderr, _("%s: out of memory\n"), progname);
+ exit(1);
+ }
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = pguser;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! conn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
if (!conn)
{
Index: src/bin/psql/command.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/command.c,v
retrieving revision 1.213
diff -c -r1.213 command.c
*** src/bin/psql/command.c 2 Jan 2010 16:57:59 -0000 1.213
--- src/bin/psql/command.c 4 Feb 2010 01:18:34 -0000
***************
*** 1213,1219 ****
* Connects to a database with given parameters. If there exists an
* established connection, NULL values will be replaced with the ones
* in the current connection. Otherwise NULL will be passed for that
! * parameter to PQsetdbLogin(), so the libpq defaults will be used.
*
* In interactive mode, if connection fails with the given parameters,
* the old connection will be kept.
--- 1213,1219 ----
* Connects to a database with given parameters. If there exists an
* established connection, NULL values will be replaced with the ones
* in the current connection. Otherwise NULL will be passed for that
! * parameter to PQconnectdbParams(), so the libpq defaults will be used.
*
* In interactive mode, if connection fails with the given parameters,
* the old connection will be kept.
***************
*** 1255,1262 ****
while (true)
{
! n_conn = PQsetdbLogin(host, port, NULL, NULL,
! dbname, user, password);
/* We can immediately discard the password -- no longer needed */
if (password)
--- 1255,1283 ----
while (true)
{
! #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] = host;
! keywords[1] = "port";
! values[1] = port;
! keywords[2] = "user";
! values[2] = user;
! keywords[3] = "password";
! values[3] = password;
! keywords[4] = "dbname";
! values[4] = dbname;
! keywords[5] = "fallback_application_name";
! values[5] = pset.progname;
! keywords[6] = NULL;
! values[6] = NULL;
!
! n_conn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
/* We can immediately discard the password -- no longer needed */
if (password)
Index: src/bin/psql/startup.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.159
diff -c -r1.159 startup.c
*** src/bin/psql/startup.c 28 Jan 2010 06:28:26 -0000 1.159
--- src/bin/psql/startup.c 4 Feb 2010 02:31:32 -0000
***************
*** 90,97 ****
char *password = NULL;
char *password_prompt = NULL;
bool new_pass;
- const char *keywords[] = {"host","port","dbname","user",
- "password","application_name",NULL};
set_pglocale_pgservice(argv[0], PG_TEXTDOMAIN("psql"));
--- 90,95 ----
***************
*** 173,192 ****
/* loop until we have a password if requested by backend */
do
{
! const char *values[] = {
! options.host,
! options.port,
! (options.action == ACT_LIST_DB &&
! options.dbname == NULL) ? "postgres" : options.dbname,
! options.username,
! password,
! pset.progname,
! NULL
! };
!
! new_pass = false;
!
! pset.db = PQconnectdbParams(keywords, values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
--- 171,201 ----
/* loop until we have a password if requested by backend */
do
{
! #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;
!
! new_pass = false;
! pset.db = PQconnectdbParams(keywords, values, true);
! free(keywords);
! free(values);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
Index: src/bin/scripts/common.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/bin/scripts/common.c,v
retrieving revision 1.38
diff -c -r1.38 common.c
*** src/bin/scripts/common.c 2 Jan 2010 16:58:00 -0000 1.38
--- src/bin/scripts/common.c 4 Feb 2010 03:54:14 -0000
***************
*** 108,115 ****
*/
do
{
new_pass = false;
! conn = PQsetdbLogin(pghost, pgport, NULL, NULL, dbname, pguser, password);
if (!conn)
{
--- 108,143 ----
*/
do
{
+ #define PARAMS_ARRAY_SIZE 7
+ const char **keywords = malloc(PARAMS_ARRAY_SIZE * sizeof(*keywords));
+ const char **values = malloc(PARAMS_ARRAY_SIZE * sizeof(*values));
+
+ if (!keywords || !values)
+ {
+ fprintf(stderr, _("%s: out of memory\n"), progname);
+ exit(1);
+ }
+
+ keywords[0] = "host";
+ values[0] = pghost;
+ keywords[1] = "port";
+ values[1] = pgport;
+ keywords[2] = "user";
+ values[2] = pguser;
+ keywords[3] = "password";
+ values[3] = password;
+ keywords[4] = "dbname";
+ values[4] = dbname;
+ keywords[5] = "fallback_application_name";
+ values[5] = progname;
+ keywords[6] = NULL;
+ values[6] = NULL;
+
new_pass = false;
! conn = PQconnectdbParams(keywords, values, true);
!
! free(keywords);
! free(values);
if (!conn)
{
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.385
diff -c -r1.385 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 28 Jan 2010 06:28:26 -0000 1.385
--- src/interfaces/libpq/fe-connect.c 4 Feb 2010 16:51:34 -0000
***************
*** 269,275 ****
PQExpBuffer errorMessage, bool use_defaults);
static PQconninfoOption *conninfo_array_parse(const char **keywords,
const char **values, PQExpBuffer errorMessage,
! bool use_defaults);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
--- 269,275 ----
PQExpBuffer errorMessage, bool use_defaults);
static PQconninfoOption *conninfo_array_parse(const char **keywords,
const char **values, PQExpBuffer errorMessage,
! bool use_defaults, int expand_dbname);
static char *conninfo_getval(PQconninfoOption *connOptions,
const char *keyword);
static void defaultNoticeReceiver(void *arg, const PGresult *res);
***************
*** 336,344 ****
* call succeeded.
*/
PGconn *
! PQconnectdbParams(const char **keywords, const char **values)
{
! PGconn *conn = PQconnectStartParams(keywords, values);
if (conn && conn->status != CONNECTION_BAD)
(void) connectDBComplete(conn);
--- 336,346 ----
* call succeeded.
*/
PGconn *
! PQconnectdbParams(const char **keywords,
! const char **values,
! int expand_dbname)
{
! PGconn *conn = PQconnectStartParams(keywords, values, expand_dbname);
if (conn && conn->status != CONNECTION_BAD)
(void) connectDBComplete(conn);
***************
*** 400,406 ****
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords, const char **values)
{
PGconn *conn;
PQconninfoOption *connOptions;
--- 402,410 ----
* See PQconnectPoll for more info.
*/
PGconn *
! PQconnectStartParams(const char **keywords,
! const char **values,
! int expand_dbname)
{
PGconn *conn;
PQconninfoOption *connOptions;
***************
*** 416,422 ****
* Parse the conninfo arrays
*/
connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage, true);
if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
--- 420,427 ----
* Parse the conninfo arrays
*/
connOptions = conninfo_array_parse(keywords, values,
! &conn->errorMessage,
! true, expand_dbname);
if (connOptions == NULL)
{
conn->status = CONNECTION_BAD;
***************
*** 3729,3744 ****
* left in errorMessage.
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
*/
static PQconninfoOption *
conninfo_array_parse(const char **keywords, const char **values,
! PQExpBuffer errorMessage, bool use_defaults)
{
char *tmp;
PQconninfoOption *options;
PQconninfoOption *option;
int i = 0;
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
if (options == NULL)
--- 3734,3786 ----
* left in errorMessage.
* Defaults are supplied (from a service file, environment variables, etc)
* for unspecified options, but only if use_defaults is TRUE.
+ *
+ * If expand_dbname is non-zero, and the value passed for keyword "dbname"
+ * contains an "=", assume it is a conninfo string and process it,
+ * overriding any previously processed conflicting keywords. Subsequent
+ * keywords will take precedence, however.
*/
static PQconninfoOption *
conninfo_array_parse(const char **keywords, const char **values,
! PQExpBuffer errorMessage, bool use_defaults,
! int expand_dbname)
{
char *tmp;
PQconninfoOption *options;
+ PQconninfoOption *str_options = NULL;
PQconninfoOption *option;
int i = 0;
+ /*
+ * If expand_dbname is non-zero, check keyword "dbname"
+ * to see if val is actually a conninfo string
+ */
+ while(expand_dbname && keywords[i])
+ {
+ const char *pname = keywords[i];
+ const char *pvalue = values[i];
+
+ /* first find "dbname" if any */
+ if (strcmp(pname, "dbname") == 0)
+ {
+ /* next look for "=" in the value */
+ if (pvalue && strchr(pvalue, '='))
+ {
+ /*
+ * Must be a conninfo string, so parse it, but do not
+ * use defaults here -- those get picked up later.
+ * We only want to override for those parameters actually
+ * passed.
+ */
+ str_options = conninfo_parse(pvalue, errorMessage, false);
+ if (str_options == NULL)
+ return NULL;
+ }
+ break;
+ }
+ ++i;
+ }
+
/* Make a working copy of PQconninfoOptions */
options = malloc(sizeof(PQconninfoOptions));
if (options == NULL)
***************
*** 3749,3754 ****
--- 3791,3797 ----
}
memcpy(options, PQconninfoOptions, sizeof(PQconninfoOptions));
+ i = 0;
/* Parse the keywords/values arrays */
while(keywords[i])
{
***************
*** 3774,3795 ****
return NULL;
}
! /*
! * Store the value
! */
! if (option->val)
! free(option->val);
! option->val = strdup(pvalue);
! if (!option->val)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! PQconninfoFree(options);
! return NULL;
! }
}
++i;
}
/*
* Stop here if caller doesn't want defaults filled in.
--- 3817,3870 ----
return NULL;
}
! /*
! * If we are on the dbname parameter, and we have a parsed
! * conninfo string, copy those parameters across, overriding
! * any existing previous settings
! */
! if (strcmp(pname, "dbname") == 0 && str_options)
! {
! PQconninfoOption *str_option;
!
! for (str_option = str_options; str_option->keyword != NULL; str_option++)
! {
! if (str_option->val != NULL)
! {
! int k;
!
! for (k = 0; options[k].keyword; k++)
! {
! if (strcmp(options[k].keyword, str_option->keyword) == 0)
! {
! if (options[k].val)
! free(options[k].val);
! options[k].val = strdup(str_option->val);
! break;
! }
! }
! }
! }
! }
! else
! {
! /*
! * Store the value, overriding previous settings
! */
! if (option->val)
! free(option->val);
! option->val = strdup(pvalue);
! if (!option->val)
! {
! printfPQExpBuffer(errorMessage,
! libpq_gettext("out of memory\n"));
! PQconninfoFree(options);
! return NULL;
! }
! }
}
++i;
}
+ PQconninfoFree(str_options);
/*
* Stop here if caller doesn't want defaults filled in.
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /opt/src/cvs/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.150
diff -c -r1.150 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 28 Jan 2010 06:28:26 -0000 1.150
--- src/interfaces/libpq/libpq-fe.h 4 Feb 2010 16:48:17 -0000
***************
*** 226,237 ****
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
! extern PGconn *PQconnectStartParams(const char **keywords, const char **values);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
! extern PGconn *PQconnectdbParams(const char **keywords, const char **values);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
--- 226,239 ----
/* make a new client connection to the backend */
/* Asynchronous (non-blocking) */
extern PGconn *PQconnectStart(const char *conninfo);
! extern PGconn *PQconnectStartParams(const char **keywords,
! const char **values, int expand_dbname);
extern PostgresPollingStatusType PQconnectPoll(PGconn *conn);
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
! extern PGconn *PQconnectdbParams(const char **keywords,
! const char **values, int expand_dbname);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
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@joeconway.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