Re: [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq

This patch came up from discussion about pg_isready.

PQconninfoParseParams is similar to PQconninfoParse but takes two
arrays like PQconnectdbParams. It essentially exposes
conninfo_array_parse().

PQconninfodefaultsMerge essentially exposes conninfo_add_defaults().
It allows you to pass a PQconninfoOption struct and it adds defaults
for all NULL values.

There are no docs yet. I assumed I would let bikeshedding ensue, and
also debate on whether we even want these first.

On Feb 3, 2013 4:16 AM, "Phil Sorber" <phil(at)omniti(dot)com> wrote:
>
> This patch came up from discussion about pg_isready.
>
> PQconninfoParseParams is similar to PQconninfoParse but takes two
> arrays like PQconnectdbParams. It essentially exposes
> conninfo_array_parse().
>
> PQconninfodefaultsMerge essentially exposes conninfo_add_defaults().
> It allows you to pass a PQconninfoOption struct and it adds defaults
> for all NULL values.
>
> There are no docs yet. I assumed I would let bikeshedding ensue, and
> also debate on whether we even want these first.

I think you forgot to attach the patch.

/Magnus

On Sun, Feb 3, 2013 at 1:37 AM, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
>
> On Feb 3, 2013 4:16 AM, "Phil Sorber" <phil(at)omniti(dot)com> wrote:
>>
>> This patch came up from discussion about pg_isready.
>>
>> PQconninfoParseParams is similar to PQconninfoParse but takes two
>> arrays like PQconnectdbParams. It essentially exposes
>> conninfo_array_parse().
>>
>> PQconninfodefaultsMerge essentially exposes conninfo_add_defaults().
>> It allows you to pass a PQconninfoOption struct and it adds defaults
>> for all NULL values.
>>
>> There are no docs yet. I assumed I would let bikeshedding ensue, and
>> also debate on whether we even want these first.
>
> I think you forgot to attach the patch.
>
> /Magnus

/me hangs head in shame :-(

Here is is...

> > On Feb 3, 2013 4:16 AM, "Phil Sorber" <phil(at)omniti(dot)com> wrote:
> >>
> >> This patch came up from discussion about pg_isready.
> >>
> >> PQconninfoParseParams is similar to PQconninfoParse but takes two
> >> arrays like PQconnectdbParams. It essentially exposes
> >> conninfo_array_parse().
> >>
> >> PQconninfodefaultsMerge essentially exposes conninfo_add_defaults().
> >> It allows you to pass a PQconninfoOption struct and it adds defaults
> >> for all NULL values.

Uh, no existing code can use this new functionality? That seems
disappointing.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
>> > On Feb 3, 2013 4:16 AM, "Phil Sorber" <phil(at)omniti(dot)com> wrote:
>> >>
>> >> This patch came up from discussion about pg_isready.
>> >>
>> >> PQconninfoParseParams is similar to PQconninfoParse but takes two
>> >> arrays like PQconnectdbParams. It essentially exposes
>> >> conninfo_array_parse().
>> >>
>> >> PQconninfodefaultsMerge essentially exposes conninfo_add_defaults().
>> >> It allows you to pass a PQconninfoOption struct and it adds defaults
>> >> for all NULL values.
>
> Uh, no existing code can use this new functionality? That seems
> disappointing.
>

I wrote this because I wanted to use it in pg_isready. I also wrote
something for pg_isready to get around not having this. I think adding
these two functions to libpq would be the better option, but wanted
something that could fix an existing issue without having to patch
libpq so late in the 9.3 development process. Actually, I think it was
you who suggested that approach.

So long answer short, there is existing code that can use this
functionality immediately.

>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

Phil Sorber wrote:
> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:

> > Uh, no existing code can use this new functionality? That seems
> > disappointing.
>
> I wrote this because I wanted to use it in pg_isready. I also wrote
> something for pg_isready to get around not having this. I think adding
> these two functions to libpq would be the better option, but wanted
> something that could fix an existing issue without having to patch
> libpq so late in the 9.3 development process. Actually, I think it was
> you who suggested that approach.

Yes, I realize that (and yes, I did). But is no code other than
pg_isready doing this? Not even the libpq URI test program?

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Phil Sorber wrote:
>> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
>
>> > Uh, no existing code can use this new functionality? That seems
>> > disappointing.
>>
>> I wrote this because I wanted to use it in pg_isready. I also wrote
>> something for pg_isready to get around not having this. I think adding
>> these two functions to libpq would be the better option, but wanted
>> something that could fix an existing issue without having to patch
>> libpq so late in the 9.3 development process. Actually, I think it was
>> you who suggested that approach.
>
> Yes, I realize that (and yes, I did). But is no code other than
> pg_isready doing this? Not even the libpq URI test program?

I think it probably would be able to benefit from this. Are you
suggesting I patch that too? I thought it was usually frowned upon to
touch random bits of working code like that. I'd be more than happy to
do it if it helps build the case for getting this added.

>
> --
> Álvaro Herrera http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services

Phil Sorber wrote:
> On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
> <alvherre(at)2ndquadrant(dot)com> wrote:
> > Phil Sorber wrote:
> >> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> wrote:
> >
> >> > Uh, no existing code can use this new functionality? That seems
> >> > disappointing.
> >>
> >> I wrote this because I wanted to use it in pg_isready. I also wrote
> >> something for pg_isready to get around not having this. I think adding
> >> these two functions to libpq would be the better option, but wanted
> >> something that could fix an existing issue without having to patch
> >> libpq so late in the 9.3 development process. Actually, I think it was
> >> you who suggested that approach.
> >
> > Yes, I realize that (and yes, I did). But is no code other than
> > pg_isready doing this? Not even the libpq URI test program?
>
> I think it probably would be able to benefit from this. Are you
> suggesting I patch that too? I thought it was usually frowned upon to
> touch random bits of working code like that. I'd be more than happy to
> do it if it helps build the case for getting this added.

Well, this qualifies as refactoring, so yeah, it helps the case.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On 04.02.2013 17:32, Alvaro Herrera wrote:
> Phil Sorber wrote:
>> On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Phil Sorber wrote:
>>>> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera<alvherre(at)2ndquadrant(dot)com> wrote:
>>>
>>>>> Uh, no existing code can use this new functionality? That seems
>>>>> disappointing.
>>>>
>>>> I wrote this because I wanted to use it in pg_isready. I also wrote
>>>> something for pg_isready to get around not having this. I think adding
>>>> these two functions to libpq would be the better option, but wanted
>>>> something that could fix an existing issue without having to patch
>>>> libpq so late in the 9.3 development process. Actually, I think it was
>>>> you who suggested that approach.
>>>
>>> Yes, I realize that (and yes, I did). But is no code other than
>>> pg_isready doing this? Not even the libpq URI test program?
>>
>> I think it probably would be able to benefit from this. Are you
>> suggesting I patch that too? I thought it was usually frowned upon to
>> touch random bits of working code like that. I'd be more than happy to
>> do it if it helps build the case for getting this added.
>
> Well, this qualifies as refactoring, so yeah, it helps the case.

I think this patch would simplift the patch to pass a connection string
to pg_basebackup and pg_receivexlog:
http://www.postgresql.org/message-id/005501cdfa45$6b0eec80$412cc580$@kommi@huawei.com.
I note that pg_dumpall also has a similar issue as pg_basebackup and
pg_receivexlog; there's no way to pass a connection string to it either.

If you could come up with a unified patch that takes care of all of
those, that would be great.

- Heikki

On Mon, Feb 11, 2013 at 4:19 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> On 04.02.2013 17:32, Alvaro Herrera wrote:
>>
>> Phil Sorber wrote:
>>>
>>> On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
>>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>>>
>>>> Phil Sorber wrote:
>>>>>
>>>>> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro
>>>>> Herrera<alvherre(at)2ndquadrant(dot)com> wrote:
>>>>
>>>>
>>>>>> Uh, no existing code can use this new functionality? That seems
>>>>>> disappointing.
>>>>>
>>>>>
>>>>> I wrote this because I wanted to use it in pg_isready. I also wrote
>>>>> something for pg_isready to get around not having this. I think adding
>>>>> these two functions to libpq would be the better option, but wanted
>>>>> something that could fix an existing issue without having to patch
>>>>> libpq so late in the 9.3 development process. Actually, I think it was
>>>>> you who suggested that approach.
>>>>
>>>>
>>>> Yes, I realize that (and yes, I did). But is no code other than
>>>> pg_isready doing this? Not even the libpq URI test program?
>>>
>>>
>>> I think it probably would be able to benefit from this. Are you
>>> suggesting I patch that too? I thought it was usually frowned upon to
>>> touch random bits of working code like that. I'd be more than happy to
>>> do it if it helps build the case for getting this added.
>>
>>
>> Well, this qualifies as refactoring, so yeah, it helps the case.
>
>
> I think this patch would simplift the patch to pass a connection string to
> pg_basebackup and pg_receivexlog:
> http://www.postgresql.org/message-id/005501cdfa45$6b0eec80$412cc580$@kommi@huawei.com.
> I note that pg_dumpall also has a similar issue as pg_basebackup and
> pg_receivexlog; there's no way to pass a connection string to it either.
>
> If you could come up with a unified patch that takes care of all of those,
> that would be great.

I will take a look.

>
> - Heikki

On Tuesday, February 12, 2013 2:49 AM Heikki Linnakangas wrote:
On 04.02.2013 17:32, Alvaro Herrera wrote:
> Phil Sorber wrote:
>> On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>> Phil Sorber wrote:
>>>> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera<alvherre(at)2ndquadrant(dot)com> wrote:
>>>

> I think this patch would simplift the patch to pass a connection string
> to pg_basebackup and pg_receivexlog:
> http://www.postgresql.org/message-id/005501cdfa45$6b0eec80$412cc580$@kommi@huawei.com.
> I note that pg_dumpall also has a similar issue as pg_basebackup and
> pg_receivexlog; there's no way to pass a connection string to it either.

I have looked into both patches and my analysis is as below:

In pg_basebackup patch, we have connection string and list of keywords (individual options specified by user),
in the current available patch it has combined connection string and list of keywords as connection string
and called PQconnectdb() which takes connection string as input.

Now the patch of Phil Sober provides 2 new API's PQconninfoParseParams(), and PQconninfodefaultsMerge(),
using these API's I can think of below way for patch "pass a connection string to pg_basebackup, ..."

1. Call existing function PQconinfoParse() with connection string input by user and get PQconninfoOption.

2. Now use the existing keywords (individual options specified by user) and extract the keywords from
PQconninfoOption structure and call new API PQconninfoParseParams() which will return PQconninfoOption.
The PQconninfoOption structure returned in this step will contain all keywords

3. Call PQconninfodefaultsMerge() to merge any default values if exist. Not sure if this step is required?

4. Extract individual keywords from PQconninfoOption structure and call PQconnectdbParams.

Is this inline with what you have in mind or you have thought of some other simpler way of using new API's?

With Regards,
Amit Kapila.

On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com> wrote:
> On Tuesday, February 12, 2013 2:49 AM Heikki Linnakangas wrote:
> On 04.02.2013 17:32, Alvaro Herrera wrote:
>> Phil Sorber wrote:
>>> On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
>>> <alvherre(at)2ndquadrant(dot)com> wrote:
>>>> Phil Sorber wrote:
>>>>> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera<alvherre(at)2ndquadrant(dot)com> wrote:
>>>>
>
>> I think this patch would simplift the patch to pass a connection string
>> to pg_basebackup and pg_receivexlog:
>> http://www.postgresql.org/message-id/005501cdfa45$6b0eec80$412cc580$@kommi@huawei.com.
>> I note that pg_dumpall also has a similar issue as pg_basebackup and
>> pg_receivexlog; there's no way to pass a connection string to it either.
>
> I have looked into both patches and my analysis is as below:
>
> In pg_basebackup patch, we have connection string and list of keywords (individual options specified by user),
> in the current available patch it has combined connection string and list of keywords as connection string
> and called PQconnectdb() which takes connection string as input.
>
> Now the patch of Phil Sober provides 2 new API's PQconninfoParseParams(), and PQconninfodefaultsMerge(),
> using these API's I can think of below way for patch "pass a connection string to pg_basebackup, ..."
>
> 1. Call existing function PQconinfoParse() with connection string input by user and get PQconninfoOption.
>
> 2. Now use the existing keywords (individual options specified by user) and extract the keywords from
> PQconninfoOption structure and call new API PQconninfoParseParams() which will return PQconninfoOption.
> The PQconninfoOption structure returned in this step will contain all keywords
>
> 3. Call PQconninfodefaultsMerge() to merge any default values if exist. Not sure if this step is required?
>
> 4. Extract individual keywords from PQconninfoOption structure and call PQconnectdbParams.
>
>
> Is this inline with what you have in mind or you have thought of some other simpler way of using new API's?
>
> With Regards,
> Amit Kapila.

I think what would be nice is an additional connect function that took
PQconninfoOption as a parameter. Or at least something that can
convert from PQconninfoOption to a connection string or keyword/value
arrays.

On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote:
> On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila <amit(dot)kapila(at)huawei(dot)com>
> wrote:
> > On Tuesday, February 12, 2013 2:49 AM Heikki Linnakangas wrote:
> > On 04.02.2013 17:32, Alvaro Herrera wrote:
> >> Phil Sorber wrote:
> >>> On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera
> >>> <alvherre(at)2ndquadrant(dot)com> wrote:
> >>>> Phil Sorber wrote:
> >>>>> On Mon, Feb 4, 2013 at 9:13 AM, Alvaro
> Herrera<alvherre(at)2ndquadrant(dot)com> wrote:
> >>>>
> >
> >> I think this patch would simplift the patch to pass a connection
> string
> >> to pg_basebackup and pg_receivexlog:
> >> http://www.postgresql.org/message-
> id/005501cdfa45$6b0eec80$412cc580$(at)kommi@huawei.com.
> >> I note that pg_dumpall also has a similar issue as pg_basebackup and
> >> pg_receivexlog; there's no way to pass a connection string to it
> either.
> >
> > I have looked into both patches and my analysis is as below:
> >
> > In pg_basebackup patch, we have connection string and list of
> keywords (individual options specified by user),
> > in the current available patch it has combined connection string and
> list of keywords as connection string
> > and called PQconnectdb() which takes connection string as input.
> >
> > Now the patch of Phil Sober provides 2 new API's
> PQconninfoParseParams(), and PQconninfodefaultsMerge(),
> > using these API's I can think of below way for patch "pass a
> connection string to pg_basebackup, ..."
> >
> > 1. Call existing function PQconinfoParse() with connection string
> input by user and get PQconninfoOption.
> >
> > 2. Now use the existing keywords (individual options specified by
> user) and extract the keywords from
> > PQconninfoOption structure and call new API
> PQconninfoParseParams() which will return PQconninfoOption.
> > The PQconninfoOption structure returned in this step will contain
> all keywords
> >
> > 3. Call PQconninfodefaultsMerge() to merge any default values if
> exist. Not sure if this step is required?
> >
> > 4. Extract individual keywords from PQconninfoOption structure and
> call PQconnectdbParams.
> >
> >
> > Is this inline with what you have in mind or you have thought of some
> other simpler way of using new API's?
> >
> > With Regards,
> > Amit Kapila.
>
> I think what would be nice is an additional connect function that took
> PQconninfoOption as a parameter. Or at least something that can
> convert from PQconninfoOption to a connection string or keyword/value
> arrays.

Yes, it would be better if we would like to use new API's, I think it can
save step-4 and some part in step-2.
I am not sure for this purpose would it be okay to introduce new Connect
API?

I also feel it will increase the scope of patch.

Heikki, would you be more specific that what in the patch you want to see
simplified.
Is the combining of keywords and connection string makes you feel the area
where patch can be improved.

With Regards,
Amit Kapila.

On 18.02.2013 06:07, Amit Kapila wrote:
> On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote:
>> On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila<amit(dot)kapila(at)huawei(dot)com>
>> wrote:
>>> Now the patch of Phil Sober provides 2 new API's
>> PQconninfoParseParams(), and PQconninfodefaultsMerge(),
>>> using these API's I can think of below way for patch "pass a
>> connection string to pg_basebackup, ..."
>>>
>>> 1. Call existing function PQconinfoParse() with connection string
>> input by user and get PQconninfoOption.
>>>
>>> 2. Now use the existing keywords (individual options specified by
>> user) and extract the keywords from
>>> PQconninfoOption structure and call new API
>> PQconninfoParseParams() which will return PQconninfoOption.
>>> The PQconninfoOption structure returned in this step will contain
>> all keywords
>>>
>>> 3. Call PQconninfodefaultsMerge() to merge any default values if
>> exist. Not sure if this step is required?
>>>
>>> 4. Extract individual keywords from PQconninfoOption structure and
>> call PQconnectdbParams.
>>>
>>> Is this inline with what you have in mind or you have thought of some
>> other simpler way of using new API's?

Yep, that's roughly what I had in mind. I don't think it's necessary to
merge defaults in step 3, but it needs to add the "replication=true" and
"dbname=replication" options.

>> I think what would be nice is an additional connect function that took
>> PQconninfoOption as a parameter. Or at least something that can
>> convert from PQconninfoOption to a connection string or keyword/value
>> arrays.
>
> Yes, it would be better if we would like to use new API's, I think it can
> save step-4 and some part in step-2.

pg_basebackup needs to add options to the array, so I don't think a new
connect function would help much. It's not a lot of code to iterate
through the PGconnInfoOption array and turn it back into keywords and
values arrays, so I'd just do that straight in the client code.

- Heikki

On Monday, February 18, 2013 1:41 PM Heikki Linnakangas wrote:
> On 18.02.2013 06:07, Amit Kapila wrote:
> > On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote:
> >> On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila<amit(dot)kapila(at)huawei(dot)com>
> >> wrote:
> >>> Now the patch of Phil Sober provides 2 new API's
> >> PQconninfoParseParams(), and PQconninfodefaultsMerge(),
> >>> using these API's I can think of below way for patch "pass a
> >> connection string to pg_basebackup, ..."
> >>>
> >>> 1. Call existing function PQconinfoParse() with connection string
> >> input by user and get PQconninfoOption.
> >>>
> >>> 2. Now use the existing keywords (individual options specified by
> >> user) and extract the keywords from
> >>> PQconninfoOption structure and call new API
> >> PQconninfoParseParams() which will return PQconninfoOption.
> >>> The PQconninfoOption structure returned in this step will
> contain
> >> all keywords
> >>>
> >>> 3. Call PQconninfodefaultsMerge() to merge any default values if
> >> exist. Not sure if this step is required?
> >>>
> >>> 4. Extract individual keywords from PQconninfoOption structure and
> >> call PQconnectdbParams.
> >>>
> >>> Is this inline with what you have in mind or you have thought of
> some
> >> other simpler way of using new API's?
>
> Yep, that's roughly what I had in mind. I don't think it's necessary to
> merge defaults in step 3, but it needs to add the "replication=true"
> and
> "dbname=replication" options.
>
> >> I think what would be nice is an additional connect function that
> took
> >> PQconninfoOption as a parameter. Or at least something that can
> >> convert from PQconninfoOption to a connection string or
> keyword/value
> >> arrays.
> >
> > Yes, it would be better if we would like to use new API's, I think it
> can
> > save step-4 and some part in step-2.
>
> pg_basebackup needs to add options to the array, so I don't think a new
> connect function would help much. It's not a lot of code to iterate
> through the PGconnInfoOption array and turn it back into keywords and
> values arrays, so I'd just do that straight in the client code.

Okay, got the point.

Phil, I will try to finish the combined patch. Is it possible for you to
complete
the documentation for the new API's.

With Regards,
Amit Kapila.

On Monday, February 18, 2013 1:41 PM Heikki Linnakangas wrote:
> On 18.02.2013 06:07, Amit Kapila wrote:
> > On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote:
> >> On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila<amit(dot)kapila(at)huawei(dot)com>
> >> wrote:
> >>> Now the patch of Phil Sober provides 2 new API's
> >> PQconninfoParseParams(), and PQconninfodefaultsMerge(),
> >>> using these API's I can think of below way for patch "pass a
> >> connection string to pg_basebackup, ..."
> >>>
> >>> 1. Call existing function PQconinfoParse() with connection string
> >> input by user and get PQconninfoOption.
> >>>
> >>> 2. Now use the existing keywords (individual options specified by
> >> user) and extract the keywords from
> >>> PQconninfoOption structure and call new API
> >> PQconninfoParseParams() which will return PQconninfoOption.
> >>> The PQconninfoOption structure returned in this step will
> contain
> >> all keywords
> >>>
> >>> 3. Call PQconninfodefaultsMerge() to merge any default values if
> >> exist. Not sure if this step is required?
> >>>
> >>> 4. Extract individual keywords from PQconninfoOption structure and
> >> call PQconnectdbParams.
> >>>
> >>> Is this inline with what you have in mind or you have thought of
> some
> >> other simpler way of using new API's?
>
> Yep, that's roughly what I had in mind. I don't think it's necessary to
> merge defaults in step 3, but it needs to add the "replication=true"
> and
> "dbname=replication" options.

I could see the advantage of calling PQconninfoParseParams() in step-2 is
that
it will remove the duplicate values by overriding the values for conflicting
keywords.
This is done in function conninfo_array_parse() which is called from
PQconninfoParseParams().
Am I right or there is any other advantage of calling
PQconninfoParseParams()?

If there is no other advantage then this is done in PQconnectdbParams()
also, so can't we avoid calling PQconninfoParseParams()?

With Regards,
Amit Kapila.

> -----Original Message-----
> From: pgsql-hackers-owner(at)postgresql(dot)org [mailto:pgsql-hackers-
> owner(at)postgresql(dot)org] On Behalf Of Amit Kapila
> Sent: Monday, February 18, 2013 6:38 PM
> To: 'Heikki Linnakangas'
> Cc: 'Phil Sorber'; 'Alvaro Herrera'; 'Magnus Hagander'; 'PostgreSQL-
> development'
> Subject: Re: [HACKERS] [PATCH] Add PQconninfoParseParams and
> PQconninfodefaultsMerge to libpq
>
> On Monday, February 18, 2013 1:41 PM Heikki Linnakangas wrote:
> > On 18.02.2013 06:07, Amit Kapila wrote:
> > > On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote:
> > >> On Sun, Feb 17, 2013 at 1:35 AM, Amit
> kapila<amit(dot)kapila(at)huawei(dot)com>
> > >> wrote:
> > >>> Now the patch of Phil Sober provides 2 new API's
> > >> PQconninfoParseParams(), and PQconninfodefaultsMerge(),
> > >>> using these API's I can think of below way for patch "pass a
> > >> connection string to pg_basebackup, ..."
> > >>>
> > >>> 1. Call existing function PQconinfoParse() with connection string
> > >> input by user and get PQconninfoOption.
> > >>>
> > >>> 2. Now use the existing keywords (individual options specified by
> > >> user) and extract the keywords from
> > >>> PQconninfoOption structure and call new API
> > >> PQconninfoParseParams() which will return PQconninfoOption.
> > >>> The PQconninfoOption structure returned in this step will
> > contain
> > >> all keywords
> > >>>
> > >>> 3. Call PQconninfodefaultsMerge() to merge any default values if
> > >> exist. Not sure if this step is required?
> > >>>
> > >>> 4. Extract individual keywords from PQconninfoOption structure
> and
> > >> call PQconnectdbParams.
> > >>>
> > >>> Is this inline with what you have in mind or you have thought of
> > some
> > >> other simpler way of using new API's?
> >
> > Yep, that's roughly what I had in mind. I don't think it's necessary
> to
> > merge defaults in step 3, but it needs to add the "replication=true"
> > and
> > "dbname=replication" options.
>
> I could see the advantage of calling PQconninfoParseParams() in step-2
> is
> that
> it will remove the duplicate values by overriding the values for
> conflicting
> keywords.
> This is done in function conninfo_array_parse() which is called from
> PQconninfoParseParams().
> Am I right or there is any other advantage of calling
> PQconninfoParseParams()?
>
> If there is no other advantage then this is done in PQconnectdbParams()
> also, so can't we avoid calling PQconninfoParseParams()?

----
> I note that pg_dumpall also has a similar issue as pg_basebackup and
> pg_receivexlog; there's no way to pass a connection string to it
> either.

I think not only pg_dumpall, but we need to add it to pg_dump.
As -C is already used option in pg_dump, I need to use something different.
I am planning to use -K as new option(available ones were
d,g,j,k,l,m,p,q,y).

I am planning to keep option same for pg_dumpall, as pg_dumpall internally
calls pg_dump with the options supplied by user.
In fact, I think we can hack the string passed to pg_dump to change the
option from -C to -K, but I am not able see if it will be way better than
using -K for both.

Suggestions?

With Regards,
Amit Kapila.

> > Tuesday, February 19, 2013 6:23 PM Amit Kapila wrote:
> > On Monday, February 18, 2013 1:41 PM Heikki Linnakangas wrote:
> > > On 18.02.2013 06:07, Amit Kapila wrote:
> > > > On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote:
> > > >> On Sun, Feb 17, 2013 at 1:35 AM, Amit
> > kapila<amit(dot)kapila(at)huawei(dot)com>
> > > >> wrote:
> > > >>> Now the patch of Phil Sober provides 2 new API's
> > > >> PQconninfoParseParams(), and PQconninfodefaultsMerge(),
> > > >>> using these API's I can think of below way for patch "pass a
> > > >> connection string to pg_basebackup, ..."
> > > >>>
> > > >>> 1. Call existing function PQconinfoParse() with connection
> string
> > > >> input by user and get PQconninfoOption.
> > > >>>
> > > >>> 2. Now use the existing keywords (individual options specified
> by
> > > >> user) and extract the keywords from
> > > >>> PQconninfoOption structure and call new API
> > > >> PQconninfoParseParams() which will return PQconninfoOption.
> > > >>> The PQconninfoOption structure returned in this step will
> > > contain
> > > >> all keywords
> > > >>>
> > > >>> 3. Call PQconninfodefaultsMerge() to merge any default values
> if
> > > >> exist. Not sure if this step is required?
> > > >>>
> > > >>> 4. Extract individual keywords from PQconninfoOption structure
> > and
> > > >> call PQconnectdbParams.
> > > >>>
> > > >>> Is this inline with what you have in mind or you have thought
> of
> > > some
> > > >> other simpler way of using new API's?
> > >
> > > Yep, that's roughly what I had in mind. I don't think it's
> necessary
> > to
> > > merge defaults in step 3, but it needs to add the
> "replication=true"
> > > and
> > > "dbname=replication" options.
> >
> > I could see the advantage of calling PQconninfoParseParams() in step-
> 2
> > is
> > that
> > it will remove the duplicate values by overriding the values for
> > conflicting
> > keywords.
> > This is done in function conninfo_array_parse() which is called from
> > PQconninfoParseParams().
> > Am I right or there is any other advantage of calling
> > PQconninfoParseParams()?
> >
> > If there is no other advantage then this is done in
> PQconnectdbParams()
> > also, so can't we avoid calling PQconninfoParseParams()?
>
> ----
> > I note that pg_dumpall also has a similar issue as pg_basebackup and
> > pg_receivexlog; there's no way to pass a connection string to it
> > either.
>
> I think not only pg_dumpall, but we need to add it to pg_dump.
> As -C is already used option in pg_dump, I need to use something
> different.
> I am planning to use -K as new option(available ones were
> d,g,j,k,l,m,p,q,y).
>
> I am planning to keep option same for pg_dumpall, as pg_dumpall
> internally
> calls pg_dump with the options supplied by user.
> In fact, I think we can hack the string passed to pg_dump to change the
> option from -C to -K, but I am not able see if it will be way better
> than
> using -K for both.

The patch for providing connection string for pg_basebackup, pg_receivexlog,
pg_dump and pg_dumpall is attached with this mail.

With Regards,
Amit Kapila.

On 20.02.2013 11:42, Amit Kapila wrote:
> The patch for providing connection string for pg_basebackup, pg_receivexlog,
> pg_dump and pg_dumpall is attached with this mail.

Thanks. Now that I look at this patch, I realize that we don't actually
need these new functions for pg_basebackup and friends after all. We
already have PQconninfoParse(), we can just use that.

pg_dump can already take a connection string:

pg_dump "dbname=postgres port=5432"

For consistency with psql and other tools, perhaps we should add a "-d"
option to pg_dump, so that you could do:

pg_dump -d "dbname=postgres port=5432"

It'd be nice to call the option -d or --dbname in all the tools. That's
a bit confusing for pg_basebackup and pg_receivexlog, as it can *not*
actually be a database name, but it would be otherwise consistent with
the other commands.

I came up with the attached three patches. The first adds -d/--dbname
option to pg_basebackup and pg_receivexlog. The second adds it to
pg_dump, per above. The third adds it to pg_dumpall.

The third patch is a bit complicated. It first parses the user-specified
connection string using PQconninfoParse, so that it can merge in some
extra keywords: user, host, password, dbname and
fallback_application_name. It then calls PQconnectdbParams with the
keyword/value pairs. After making the initial connection to postgres or
template1 database, it calls PQconninfo() to again extract the
keyword/value pairs in effect in the connection, and constructs a new
connection string from them. That new connection string is then passed
to pg_dump on the command line, with the database name appended to it.

That seems to work, although it's perhaps a bit Rube Goldbergian. One
step of deparsing and parsing could be avoided by keeping the
keyword/value pairs from the first PQconninfoParse() call, instead of
constructing them again with PQconninfo(). I'll experiment with that
tomorrow.

The docs need some improvement. In those commands where you can't pass a
database name to the -d/--dbname option, only a connection string, I
kept your wording in the docs. But it ought to explain the seemingly
strange name for the option, and more. I'll take another whack at that
tomorrow as well.

Where does this leave the PQconninfoParseParams/PQconninfodefaultsMerge
patch? I'm not sure. Somehow I thought it would be necessary for this
work, but it wasn't. I didn't remember that we already have
PQconninfoParse() function, which was enough. So, what's the use case
for those functions?

- Heikki

On Wed, Feb 20, 2013 at 2:16 PM, Heikki Linnakangas
<hlinnakangas(at)vmware(dot)com> wrote:
> Where does this leave the PQconninfoParseParams/PQconninfodefaultsMerge
> patch? I'm not sure. Somehow I thought it would be necessary for this work,
> but it wasn't. I didn't remember that we already have PQconninfoParse()
> function, which was enough. So, what's the use case for those functions?
>

I don't think that there is an immediate case. I still think they are
useful, and would be more useful if we had some other functions that
took PQconninfoOption. But the original reason for their being has
been circumvented and I think we should just push them off to next
release commit fest and discuss them then.

> - Heikki

On Thursday, February 21, 2013 12:46 AM Heikki Linnakangas wrote:
> On 20.02.2013 11:42, Amit Kapila wrote:
> > The patch for providing connection string for pg_basebackup,
> > pg_receivexlog, pg_dump and pg_dumpall is attached with this mail.
>
> Thanks. Now that I look at this patch, I realize that we don't actually
> need these new functions for pg_basebackup and friends after all. We
> already have PQconninfoParse(), we can just use that.
>
> pg_dump can already take a connection string:
>
> pg_dump "dbname=postgres port=5432"
>
> For consistency with psql and other tools, perhaps we should add a "-d"
> option to pg_dump, so that you could do:
>
> pg_dump -d "dbname=postgres port=5432"
>
> It'd be nice to call the option -d or --dbname in all the tools. That's
> a bit confusing for pg_basebackup and pg_receivexlog, as it can *not*
> actually be a database name, but it would be otherwise consistent with
> the other commands.
>
>
> I came up with the attached three patches. The first adds -d/--dbname
> option to pg_basebackup and pg_receivexlog.

Patch-1 is working fine.

>The second adds it to
> pg_dump, per above. The third adds it to pg_dumpall.
>
> The third patch is a bit complicated. It first parses the user-
> specified connection string using PQconninfoParse, so that it can merge
> in some extra keywords: user, host, password, dbname and
> fallback_application_name. It then calls PQconnectdbParams with the
> keyword/value pairs. After making the initial connection to postgres or
> template1 database, it calls PQconninfo() to again extract the
> keyword/value pairs in effect in the connection, and constructs a new
> connection string from them. That new connection string is then passed
> to pg_dump on the command line, with the database name appended to it.
>
> That seems to work, although it's perhaps a bit Rube Goldbergian. One
> step of deparsing and parsing could be avoided by keeping the
> keyword/value pairs from the first PQconninfoParse() call, instead of
> constructing them again with PQconninfo(). I'll experiment with that
> tomorrow.

I think this whole new logic in pg_dumpall is needed because in
pg_dump(Patch-2),
we have used dbname for new option.
In pg_dump, if for new option (-d) we use new variable conn_str similar as
we used at other places and change
the ConnectDatabase() in file pg_backup_db.c similar to GetConnection of
pg_basebackup, we might not
need the new logic of deparsing and parsing in pg_dumpall.

Am I missing something or do you feel this will be more cumbersome than
current Patch-2 & Patch-3?

With Regards,
Amit Kapila.

On 21.02.2013 16:09, Amit Kapila wrote:
> On Thursday, February 21, 2013 12:46 AM Heikki Linnakangas wrote:
>> The second adds it to
>> pg_dump, per above. The third adds it to pg_dumpall.
>>
>> The third patch is a bit complicated. It first parses the user-
>> specified connection string using PQconninfoParse, so that it can merge
>> in some extra keywords: user, host, password, dbname and
>> fallback_application_name. It then calls PQconnectdbParams with the
>> keyword/value pairs. After making the initial connection to postgres or
>> template1 database, it calls PQconninfo() to again extract the
>> keyword/value pairs in effect in the connection, and constructs a new
>> connection string from them. That new connection string is then passed
>> to pg_dump on the command line, with the database name appended to it.
>>
>> That seems to work, although it's perhaps a bit Rube Goldbergian. One
>> step of deparsing and parsing could be avoided by keeping the
>> keyword/value pairs from the first PQconninfoParse() call, instead of
>> constructing them again with PQconninfo(). I'll experiment with that
>> tomorrow.

I did the above, keeping the keyword/value pairs from the
PQconninfoParse() step instead.

> I think this whole new logic in pg_dumpall is needed because in
> pg_dump(Patch-2),
> we have used dbname for new option.
> In pg_dump, if for new option (-d) we use new variable conn_str similar as
> we used at other places and change
> the ConnectDatabase() in file pg_backup_db.c similar to GetConnection of
> pg_basebackup, we might not
> need the new logic of deparsing and parsing in pg_dumpall.

Yeah, could be. However, it's good to call the option -d for the sake of
consistency. Also, there would be some confusion if there were two ways
to pass a connection string (we'd stil have to support passing a
connection string as the database name, for backwards-compatibility).

I've committed those patches, with some further changes. If you have the
time, please take another look at the committed patches. Thanks!

- Heikki

On Monday, February 25, 2013 11:26 PM Heikki Linnakangas wrote:
> On 21.02.2013 16:09, Amit Kapila wrote:
> > On Thursday, February 21, 2013 12:46 AM Heikki Linnakangas wrote:
> I've committed those patches, with some further changes. If you have
> the
> time, please take another look at the committed patches. Thanks!

1. For pg_dumpall, incase user gives dbname in connection string, for ex.
"user=amit dbname=db_regress port=5434"
it will be ignored and postgres (default database name) will be used.

2. For pg_basebackup, no need to check if(conn_opts) before PQconninfoFree
as it will internally take care of
NULL. This will not cause any harm, just an observation.

With Regards,
Amit Kapila

On 26.02.2013 09:06, Amit Kapila wrote:
> On Monday, February 25, 2013 11:26 PM Heikki Linnakangas wrote:
>> On 21.02.2013 16:09, Amit Kapila wrote:
>>> On Thursday, February 21, 2013 12:46 AM Heikki Linnakangas wrote:
>> I've committed those patches, with some further changes. If you have
>> the
>> time, please take another look at the committed patches. Thanks!
>
> 1. For pg_dumpall, incase user gives dbname in connection string, for ex.
> "user=amit dbname=db_regress port=5434"
> it will be ignored and postgres (default database name) will be used.

Yeah, that is mentioned in the docs. The other option would be to use
the database name from the connection string for the initial connection,
ie. the same as specifying a database name with the -l option. Yet
another option is to throw an error if database name was given. If we
make it an error, then we should make it an error in pg_basebackup and
pg_receivexlog too. I'm all ears for opinions on this.

- Heikki

On Tuesday, February 26, 2013 1:36 PM Heikki Linnakangas wrote:
> On 26.02.2013 09:06, Amit Kapila wrote:
> > On Monday, February 25, 2013 11:26 PM Heikki Linnakangas wrote:
> >> On 21.02.2013 16:09, Amit Kapila wrote:
> >>> On Thursday, February 21, 2013 12:46 AM Heikki Linnakangas wrote:
> >> I've committed those patches, with some further changes. If you have
> >> the
> >> time, please take another look at the committed patches. Thanks!
> >
> > 1. For pg_dumpall, incase user gives dbname in connection string, for
> ex.
> > "user=amit dbname=db_regress port=5434"
> > it will be ignored and postgres (default database name) will be
> used.
>
> Yeah, that is mentioned in the docs.

Sorry, I have missed that part, this just came to me when I was looking at
the code.

>The other option would be to use
> the database name from the connection string for the initial
> connection,
> ie. the same as specifying a database name with the -l option.

This is what come to my mind initially, as if it is allowed with -l option
it is better if the same should be allowed through connection string as
well.
We are ignoring for pg_basebackup as well, but in pg_basebackup there is no
way for user to
specify database name.

With Regards,
Amit Kapila.