Thread: 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
Attachment
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.orghttp://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
Attachment
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
Attachment
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.orghttp://dalibo.com