Re: 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(dot)linnakangas(at)enterprisedb(dot)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(at)systemguards(dot)com(dot)ec> writes:
> On Mon, Jul 6, 2009 at 10:00 AM, Heikki
> Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)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(at)systemguards(dot)com(dot)ec> wrote:
> On Mon, Jul 6, 2009 at 10:00 AM, Heikki
> Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)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

Jaime Casanova wrote:
> On Thu, Sep 10, 2009 at 12:01 AM, Jaime Casanova
> <jcasanov(at)systemguards(dot)com(dot)ec> wrote:
>> On Mon, Jul 6, 2009 at 10:00 AM, Heikki
>> Linnakangas<heikki(dot)linnakangas(at)enterprisedb(dot)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(at)esilo(dot)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(at)systemguards(dot)com(dot)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(at)esilo(dot)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."
>

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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)esilo(dot)com> wrote:
> Jaime Casanova wrote:
>>
>> On Mon, Sep 14, 2009 at 1:34 PM, Andrew Chernow <ac(at)esilo(dot)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."
>>
>
> 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(at)esilo(dot)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(at)systemguards(dot)com(dot)ec> writes:
> On Mon, Sep 14, 2009 at 1:55 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)systemguards(dot)com(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > Jaime Casanova <jcasanov(at)systemguards(dot)com(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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(at)gmx(dot)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