Re: Patch: libpq new connect function (PQconnectParams)

I'm reviewing the patch posted here:
http://archives.postgresql.org/pgsql-hackers/2010-01/msg01579.php
for this commitfest item:
https://commitfest.postgresql.org/action/patch_view?id=259

Patch attached - a few minor changes:
-------------------------------------
1) Updated to apply cleanly against cvs tip
2) Improved comments
3) Moved much of what was in PQconnectStartParams() to a new
conninfo_array_parse() to be more consistent with existing code

Questions/comments:
-------------------
a) Do we want an analog to PQconninfoParse(), e.g.
PQconninfoParseParams()? If not, it isn't worth keeping use_defaults
as an argument to conninfo_array_parse().
b) I refrained from further consolidation even though there is room.
For example, I considered leaving only the real parsing code in
conninfo_parse(), and having it return keywords and values arrays.
If we did that, the rest of the code could be modified to accept
keywords and values instead of conninfo, and therefore shared. I was
concerned about the probably small performance hit to the existing
code path. Thoughts?
c) Obviously I liked the "two-arrays approach" better -- any objections
to that?

Thanks,

Joe

Le 26/01/2010 00:04, Joe Conway a écrit :
> I'm reviewing the patch posted here:
> http://archives.postgresql.org/pgsql-hackers/2010-01/msg01579.php
> for this commitfest item:
> https://commitfest.postgresql.org/action/patch_view?id=259
>

First, thanks for reviewing my patch.

> Patch attached - a few minor changes:
> -------------------------------------
> 1) Updated to apply cleanly against cvs tip

Sorry about this. I already updated it twice. I didn't think a new
update was needed.

> 2) Improved comments

Sure.

> 3) Moved much of what was in PQconnectStartParams() to a new
> conninfo_array_parse() to be more consistent with existing code

You're right. It also makes the code more readable and understandable. I
should have done that.

> Questions/comments:
> -------------------
> a) Do we want an analog to PQconninfoParse(), e.g.
> PQconninfoParseParams()? If not, it isn't worth keeping use_defaults
> as an argument to conninfo_array_parse().

No, I don't think so. I can't find a use case for it.

> b) I refrained from further consolidation even though there is room.
> For example, I considered leaving only the real parsing code in
> conninfo_parse(), and having it return keywords and values arrays.
> If we did that, the rest of the code could be modified to accept
> keywords and values instead of conninfo, and therefore shared. I was
> concerned about the probably small performance hit to the existing
> code path. Thoughts?
> c) Obviously I liked the "two-arrays approach" better -- any objections
> to that?

No objection. I prefer the other one, but it's just not that important.

I didn't put any documentation before knowing which one will be choosen.
So we still need to work on the manual.

Thanks again.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On 01/25/2010 03:21 PM, Guillaume Lelarge wrote:
> I didn't put any documentation before knowing which one will be choosen.
> So we still need to work on the manual.

Please send the documentation as a separate patch. Once I have that I
will commit the posted patch, barring any objections in the meantime.

Joe

Le 26/01/2010 19:43, Joe Conway a écrit :
> On 01/25/2010 03:21 PM, Guillaume Lelarge wrote:
>> I didn't put any documentation before knowing which one will be choosen.
>> So we still need to work on the manual.
>
> Please send the documentation as a separate patch. Once I have that I
> will commit the posted patch, barring any objections in the meantime.
>

You'll find it attached with this mail. Please read it carefully, my
written english is not that good.

Thanks.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com

On 01/26/2010 02:55 PM, Guillaume Lelarge wrote:
> Le 26/01/2010 19:43, Joe Conway a écrit :
>> On 01/25/2010 03:21 PM, Guillaume Lelarge wrote:
>>> I didn't put any documentation before knowing which one will be choosen.
>>> So we still need to work on the manual.
>>
>> Please send the documentation as a separate patch. Once I have that I
>> will commit the posted patch, barring any objections in the meantime.
>
> You'll find it attached with this mail. Please read it carefully, my
> written english is not that good.

Final committed patch attached.

One last code correction -- in psql/startup.c the original patch defines
the keywords array in the body of the code, rather than at the top of
the block.

Minor improvements ( hopefully ;-)) to the documentation as well.

Joe

Le 28/01/2010 07:32, Joe Conway a écrit :
> On 01/26/2010 02:55 PM, Guillaume Lelarge wrote:
>> Le 26/01/2010 19:43, Joe Conway a écrit :
>>> On 01/25/2010 03:21 PM, Guillaume Lelarge wrote:
>>>> I didn't put any documentation before knowing which one will be choosen.
>>>> So we still need to work on the manual.
>>>
>>> Please send the documentation as a separate patch. Once I have that I
>>> will commit the posted patch, barring any objections in the meantime.
>>
>> You'll find it attached with this mail. Please read it carefully, my
>> written english is not that good.
>
> Final committed patch attached.
>
> One last code correction -- in psql/startup.c the original patch defines
> the keywords array in the body of the code, rather than at the top of
> the block.
>
> Minor improvements ( hopefully ;-)) to the documentation as well.
>

Thanks a lot.

--
Guillaume.
http://www.postgresqlfr.org
http://dalibo.com