new version of PQconnectdb was:(Re: [HACKERS] Determining client_encoding from client locale)
On Mon, Jul 6, 2009 at 10:00 AM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:
Could we
have a version of PQconnectdb() with an API more suited for setting the
params programmatically? The PQsetdbLogin() approach doesn't scale as
parameters are added/removed in future versions, but we could have
something like this:PGconn *PQconnectParams(const char **params)
Where "params" is an array with an even number of parameters, forming
key/value pairs. Usage example:
[...]
Another idea is to use an array of PQconninfoOption structs:
PQconn *PQconnectParams(PQconninfoOption *params);
this sounds like a good idea, specially if we add new parameters to
the conninfo string and want postgresql's client applications to use
them.
any one have a preference here?
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
PGconn *PQconnectParams(const char **params)
Where "params" is an array with an even number of parameters, forming
key/value pairs. Usage example:
Maybe use the term properties (props for short) or options instead of params?
Params is already in heavy use. How about PQconnectProps(...) or
PQconnectOptions(...)?
Another idea is to use an array of PQconninfoOption structs:
PQconn *PQconnectParams(PQconninfoOption *params);
this sounds like a good idea, specially if we add new parameters to
Here's another idea, parallel arrays:
PGconn *PQconnectProps(const char **names, const char **values);
PGconn *PQconnectOptions(const char **names, const char **values);
To build on the struct idea, maybe PGprop or PGoption instead of
PQconninfoOption. Also, add an argument specifying the number of props/options.
PGconn *PQconnectProps(const PGprop *props, int nProps);
PGconn *PQconnectOptions(const PGoption *options, int nOptions);
any one have a preference here?
I like the struct approach. I personally prefer specifying the element count of
an array, rather than using a NULL terminating element.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
On Mon, Jul 6, 2009 at 10:00 AM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:Could we
have a version of PQconnectdb() with an API more suited for setting the
params programmatically? The PQsetdbLogin() approach doesn't scale as
parameters are added/removed in future versions, but we could have
something like this:PGconn *PQconnectParams(const char **params)
Where "params" is an array with an even number of parameters, forming
key/value pairs. Usage example:[...]
Another idea is to use an array of PQconninfoOption structs:
PQconn *PQconnectParams(PQconninfoOption *params);
this sounds like a good idea, specially if we add new parameters to
the conninfo string and want postgresql's client applications to use
them.
any one have a preference here?
Let's leave PQconninfoOption out of this --- it's bizarrely
overcomplicated for what we need here. For example, it would not be
clear to a user which fields of a PQconninfoOption he actually needed
to set up for this purpose.
The alternative I would consider is two parallel arrays, one for
keywords and one for values:
PGconn *PQconnectParams(const char **keywords, const char **values)
on the grounds that the keyword set is likely to be a true constant
whereas the values won't be. But it's hard to be sure which way is
actually more convenient without having tried coding some likely
calling scenarios both ways.
I don't have much of a preference for count vs. terminating null
--- I could see either one being easier.regards, tom lane
On Thu, Sep 10, 2009 at 12:01 AM, Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:
On Mon, Jul 6, 2009 at 10:00 AM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:Could we
have a version of PQconnectdb() with an API more suited for setting the
params programmatically? The PQsetdbLogin() approach doesn't scale as
parameters are added/removed in future versions, but we could have
something like this:PGconn *PQconnectParams(const char **params)
Where "params" is an array with an even number of parameters, forming
key/value pairs. Usage example:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.
i put the new function at the end of the exports.txt file, there's a
reason to renumber the exports to put it at the beginning with the
other PQconnectdb function?
this patch still lacks documentation, i will add it in the next days
but want to know if you have any comments about this...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
Attachments:
PQconnectdbParams.patchtext/x-patch; charset=US-ASCII; name=PQconnectdbParams.patchDownload
Index: src/bin/psql/command.c
===================================================================
RCS file: /home/postgres/pgrepo/pgsql/src/bin/psql/command.c,v
retrieving revision 1.206
diff -c -r1.206 command.c
*** src/bin/psql/command.c 11 Jun 2009 14:49:07 -0000 1.206
--- src/bin/psql/command.c 14 Sep 2009 17:34:00 -0000
***************
*** 1239,1246 ****
while (true)
{
! n_conn = PQsetdbLogin(host, port, NULL, NULL,
! dbname, user, password);
/* We can immediately discard the password -- no longer needed */
if (password)
--- 1239,1254 ----
while (true)
{
! const char *params[] = {
! "host", host,
! "port", port,
! "dbname", dbname,
! "user", user,
! "password", password,
! NULL, NULL
! };
!
! n_conn = PQconnectdbParams(params);
/* We can immediately discard the password -- no longer needed */
if (password)
Index: src/bin/psql/startup.c
===================================================================
RCS file: /home/postgres/pgrepo/pgsql/src/bin/psql/startup.c,v
retrieving revision 1.156
diff -c -r1.156 startup.c
*** src/bin/psql/startup.c 5 Apr 2009 04:19:58 -0000 1.156
--- src/bin/psql/startup.c 14 Sep 2009 17:33:43 -0000
***************
*** 171,181 ****
/* loop until we have a password if requested by backend */
do
{
new_pass = false;
! pset.db = PQsetdbLogin(options.host, options.port, NULL, NULL,
! options.action == ACT_LIST_DB && options.dbname == NULL ?
! "postgres" : options.dbname,
! options.username, password);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
--- 171,189 ----
/* loop until we have a password if requested by backend */
do
{
+ const char *params[] = {
+ "host", options.host,
+ "port", options.port,
+ "dbname", (options.action == ACT_LIST_DB &&
+ options.dbname == NULL) ? "postgres" : options.dbname,
+ "user", options.username,
+ "password", password,
+ NULL, NULL
+ };
+
new_pass = false;
!
! pset.db = PQconnectdbParams(params);
if (PQstatus(pset.db) == CONNECTION_BAD &&
PQconnectionNeedsPassword(pset.db) &&
Index: src/interfaces/libpq/exports.txt
===================================================================
RCS file: /home/postgres/pgrepo/pgsql/src/interfaces/libpq/exports.txt,v
retrieving revision 1.23
diff -c -r1.23 exports.txt
*** src/interfaces/libpq/exports.txt 31 Mar 2009 01:41:27 -0000 1.23
--- src/interfaces/libpq/exports.txt 14 Sep 2009 17:33:03 -0000
***************
*** 153,155 ****
--- 153,156 ----
PQfireResultCreateEvents 151
PQconninfoParse 152
PQinitOpenSSL 153
+ PQconnectdbParams 154
Index: src/interfaces/libpq/fe-connect.c
===================================================================
RCS file: /home/postgres/pgrepo/pgsql/src/interfaces/libpq/fe-connect.c,v
retrieving revision 1.376
diff -c -r1.376 fe-connect.c
*** src/interfaces/libpq/fe-connect.c 24 Jul 2009 17:58:31 -0000 1.376
--- src/interfaces/libpq/fe-connect.c 14 Sep 2009 17:34:49 -0000
***************
*** 211,216 ****
--- 211,219 ----
"GSS-library", "", 7}, /* sizeof("gssapi") = 7 */
#endif
+ {"appname", NULL, NULL, NULL,
+ "Client-application", "", 45},
+
/* Terminating entry --- MUST BE LAST */
{NULL, NULL, NULL, NULL,
NULL, NULL, 0}
***************
*** 283,288 ****
--- 286,333 ----
*/
/*
+ * PQconnectdbParams
+ */
+ PGconn *
+ PQconnectdbParams(const char * const *params)
+ {
+ PGconn *ret;
+ PQExpBufferData buf;
+
+ initPQExpBuffer(&buf);
+
+ while(*params)
+ {
+ const char *option = params[0];
+ const char *value = params[1];
+
+ if (value != NULL)
+ {
+ /* write option name */
+ appendPQExpBuffer(&buf, "%s = '", option);
+ /* write value, escaping single quotes and backslashes */
+ while(*value)
+ {
+ if (*value == '\'' || *value == '\\')
+ appendPQExpBufferChar(&buf, '\\');
+ appendPQExpBufferChar(&buf, *(value++));
+ }
+
+ appendPQExpBufferStr(&buf, "' ");
+ }
+
+ params+=2;
+ }
+
+ ret = PQconnectdb(buf.data);
+
+ termPQExpBuffer(&buf);
+
+ return ret;
+
+ }
+
+ /*
* PQconnectdb
*
* establishes a connection to a postgres backend through the postmaster
Index: src/interfaces/libpq/libpq-fe.h
===================================================================
RCS file: /home/postgres/pgrepo/pgsql/src/interfaces/libpq/libpq-fe.h,v
retrieving revision 1.147
diff -c -r1.147 libpq-fe.h
*** src/interfaces/libpq/libpq-fe.h 11 Jun 2009 14:49:14 -0000 1.147
--- src/interfaces/libpq/libpq-fe.h 14 Sep 2009 17:34:28 -0000
***************
*** 230,235 ****
--- 230,236 ----
/* Synchronous (blocking) */
extern PGconn *PQconnectdb(const char *conninfo);
+ extern PGconn *PQconnectdbParams(const char * const *params);
extern PGconn *PQsetdbLogin(const char *pghost, const char *pgport,
const char *pgoptions, const char *pgtty,
const char *dbName,
Jaime Casanova wrote:
On Thu, Sep 10, 2009 at 12:01 AM, Jaime Casanova
<jcasanov@systemguards.com.ec> wrote:On Mon, Jul 6, 2009 at 10:00 AM, Heikki
Linnakangas<heikki.linnakangas@enterprisedb.com> wrote:Could we
have a version of PQconnectdb() with an API more suited for setting the
params programmatically? The PQsetdbLogin() approach doesn't scale as
parameters are added/removed in future versions, but we could have
something like this:PGconn *PQconnectParams(const char **params)
Where "params" is an array with an even number of parameters, forming
key/value pairs. Usage example:i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.
I was following this and never saw any firm decision on the prototype
for this function. Although, I can say the single argument version did
not appear to win any votes.
The below posts agreed on a two argument version of parallel arrays
(keywords, values):
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00533.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00559.php
There is also the idea of passing an array of structs floating around,
NULL terminated list or include an additional argument specifying
element count.
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
On Mon, Sep 14, 2009 at 1:34 PM, Andrew Chernow <ac@esilo.com> wrote:
Jaime Casanova wrote:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.
[...]
The below posts agreed on a two argument version of parallel arrays
(keywords, values):http://archives.postgresql.org/pgsql-hackers/2009-09/msg00533.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00559.php
actually, Tom said: "it's hard to be sure which way is
actually more convenient without having tried coding some likely
calling scenarios both ways."
so i tried one scenario. :) do you think is worth the trouble make the
other approach? i could make the patch if someone is interested...
personally, i think it will cause more problems than solve because you
have to be sure your arrays have relationship between them...
There is also the idea of passing an array of structs floating around, NULL
terminated list or include an additional argument specifying element count.
one more variable to the equation, more innecesary complexity and
another source of errors, IMO...
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
i put the new function at the end of the exports.txt file, there's a
reason to renumber the exports to put it at the beginning with the
other PQconnectdb function?
Exports.txt numbers do not change. EVER.
regards, tom lane
Jaime Casanova wrote:
On Mon, Sep 14, 2009 at 1:34 PM, Andrew Chernow <ac@esilo.com> wrote:
Jaime Casanova wrote:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.[...]
The below posts agreed on a two argument version of parallel arrays
(keywords, values):http://archives.postgresql.org/pgsql-hackers/2009-09/msg00533.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00559.phpactually, Tom said: "it's hard to be sure which way is
actually more convenient without having tried coding some likely
calling scenarios both ways."
Aahhh, correct you are Daniel son :)
personally, i think it will cause more problems than solve because you
have to be sure your arrays have relationship between them...
A strict relationship exists either way.
There is also the idea of passing an array of structs floating around, NULL
terminated list or include an additional argument specifying element count.one more variable to the equation, more innecesary complexity and
another source of errors, IMO...
one more variable or one more element, both of which cause problems if
omitted/incorrect.
const char *params[] =
{"host", "blah.com", "port", "6262", NULL, NULL};
// compiler enforces relationship
const PGopotion opts[] =
{{"host", "blah.com"}, {"port", "6262"}, {NULL, NULL}};
IMHO, the struct approach seems like a cleaner solution.
Any chance of using a term other than "params"? Maybe "options" or
"props"?
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
On Mon, Sep 14, 2009 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
i put the new function at the end of the exports.txt file, there's a
reason to renumber the exports to put it at the beginning with the
other PQconnectdb function?Exports.txt numbers do not change. EVER.
i didn't find any info about it, not even in the sources... should we
document that we need to put some functions in that file and for what
reasons?
actually, i was very confused when the psql fails to compile until i
understood i need to put the function in that file
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
On Mon, Sep 14, 2009 at 2:20 PM, Andrew Chernow <ac@esilo.com> wrote:
Jaime Casanova wrote:
On Mon, Sep 14, 2009 at 1:34 PM, Andrew Chernow <ac@esilo.com> wrote:
Jaime Casanova wrote:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.[...]
The below posts agreed on a two argument version of parallel arrays
(keywords, values):http://archives.postgresql.org/pgsql-hackers/2009-09/msg00533.php
http://archives.postgresql.org/pgsql-hackers/2009-09/msg00559.phpactually, Tom said: "it's hard to be sure which way is
actually more convenient without having tried coding some likely
calling scenarios both ways."Aahhh, correct you are Daniel son :)
??? don't understand you ???
personally, i think it will cause more problems than solve because you
have to be sure your arrays have relationship between them...A strict relationship exists either way.
[...]
IMHO, the struct approach seems like a cleaner solution.
i agree
Any chance of using a term other than "params"? Maybe "options" or "props"?
i don't have any problems with "options"
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
actually, Tom said: "it's hard to be sure which way is
actually more convenient without having tried coding some likely
calling scenarios both ways."Aahhh, correct you are Daniel son :)
??? don't understand you ???
From the movie "karate kid"; oopps, should be Daniel San. I was trying
to be cute but that apparently failed :(
--
Andrew Chernow
eSilo, LLC
every bit counts
http://www.esilo.com/
On Mon, Sep 14, 2009 at 3:31 PM, Andrew Chernow <ac@esilo.com> wrote:
actually, Tom said: "it's hard to be sure which way is
actually more convenient without having tried coding some likely
calling scenarios both ways."Aahhh, correct you are Daniel son :)
??? don't understand you ???
From the movie "karate kid"; oopps, should be Daniel San.
ah! got it... ;)
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
On Mon, Sep 14, 2009 at 1:55 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Exports.txt numbers do not change. EVER.
i didn't find any info about it, not even in the sources... should we
document that we need to put some functions in that file and for what
reasons?
Every function that is meant to be exported from libpq.
regards, tom lane
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.
I got around to looking at the actual patch a bit. I hadn't understood
why it was so small, but now I do: it's implemented as a wrapper around
PQconnectdb. That is, it takes the given keywords and values, and
builds a conninfo string, which PQconnectdb then has to pull apart
again. This seems, well, dumb. I'll admit that the wasted cycles are
probably not much compared to all the other costs of establishing a
connection. But it means that you're still exposed to all the other
limitations of PQconnectdb, eg any possible bugs or restrictions in the
quoting/escaping logic. It seems to me that a non-bogus patch for this
would involve refactoring the code within PQconnectdb so that the
keywords and values could be plugged in directly without the escaping
and de-escaping logic. It doesn't look that hard to pull apart
conninfo_parse into two or three functions that would support this.
Another reason why it needs refactoring is that this way doesn't expose
all the functionality that ought to be available; in particular not the
ability to do an async connection while passing the parameters in this
style. You need analogs to PQconnectStart and PQconnectPoll too.
(Actually I guess the existing PQconnectPoll would work fine, but you
definitely need an analog to PQconnectStart.)
regards, tom lane
On Wed, Sep 23, 2009 at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.I got around to looking at the actual patch a bit. I hadn't understood
why it was so small, but now I do: it's implemented as a wrapper around
PQconnectdb. That is, it takes the given keywords and values, and
builds a conninfo string, which PQconnectdb then has to pull apart
again. This seems, well, dumb. I'll admit that the wasted cycles are
probably not much compared to all the other costs of establishing a
connection. But it means that you're still exposed to all the other
limitations of PQconnectdb, eg any possible bugs or restrictions in the
quoting/escaping logic. It seems to me that a non-bogus patch for this
would involve refactoring the code within PQconnectdb so that the
keywords and values could be plugged in directly without the escaping
and de-escaping logic. It doesn't look that hard to pull apart
conninfo_parse into two or three functions that would support this.Another reason why it needs refactoring is that this way doesn't expose
all the functionality that ought to be available; in particular not the
ability to do an async connection while passing the parameters in this
style. You need analogs to PQconnectStart and PQconnectPoll too.
(Actually I guess the existing PQconnectPoll would work fine, but you
definitely need an analog to PQconnectStart.)
Based on this review, it sounds like this patch will need a major
rewrite before it can be seriously reviewed. Given that, I think it
makes sense to postpone this to the next CommitFest, so I am going to
mark it as Returned with Feedback.
...Robert
On Sun, 2009-09-27 at 21:49 -0400, Robert Haas wrote:
On Wed, Sep 23, 2009 at 3:26 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Jaime Casanova <jcasanov@systemguards.com.ec> writes:
i extracted the functions to connect that Heikki put on psql in his
patch for determining client_encoding from client locale and put it in
libpq so i follow the PQconnectdbParams(* params[]) approach.I got around to looking at the actual patch a bit. I hadn't understood
why it was so small, but now I do: it's implemented as a wrapper around
PQconnectdb. That is, it takes the given keywords and values, and
builds a conninfo string, which PQconnectdb then has to pull apart
again.
Based on this review, it sounds like this patch will need a major
rewrite before it can be seriously reviewed. Given that, I think it
makes sense to postpone this to the next CommitFest, so I am going to
mark it as Returned with Feedback.
Is anyone planning to do further work on this? This appears to be
blocking the client_encoding=auto feature.
Peter Eisentraut <peter_e@gmx.net> writes:
Is anyone planning to do further work on this? This appears to be
blocking the client_encoding=auto feature.
Huh? Why would there be any linkage? This is just offering an
alternative way to set connection options, it has nothing to do
with what the set of options is.
regards, tom lane
On Tue, 2009-11-03 at 09:32 -0500, Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Is anyone planning to do further work on this? This appears to be
blocking the client_encoding=auto feature.Huh? Why would there be any linkage? This is just offering an
alternative way to set connection options, it has nothing to do
with what the set of options is.
Right, but we got stuck when we realized that we would need to switch
psql's connection handling from PQsetdb to PQconnectdb, which would be
annoying. And at the moment everyone involved appears to be waiting on
this hypothetical newer version of PQconnectdb to make the original
patch easier.
Tom Lane wrote:
Peter Eisentraut <peter_e@gmx.net> writes:
Is anyone planning to do further work on this? This appears to be
blocking the client_encoding=auto feature.Huh? Why would there be any linkage? This is just offering an
alternative way to set connection options, it has nothing to do
with what the set of options is.
The client_encoding=auto feature would use the new function in psql to
set the client_encoding to 'auto'. Otherwise it needs to construct a
properly quoted connection string and pass it to the existing
PQconnectdb, which is a bit laborious.
--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com
On Tue, Nov 3, 2009 at 6:31 AM, Peter Eisentraut <peter_e@gmx.net> wrote:
Is anyone planning to do further work on this? This appears to be
blocking the client_encoding=auto feature.
yes, i'm planning to make an attempt to do it as soon as i get some
time... but if you think it's important enough please go for it
--
Atentamente,
Jaime Casanova
Soporte y capacitación de PostgreSQL
Asesoría y desarrollo de sistemas
Guayaquil - Ecuador
Cel. +59387171157