Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 kapilaamit.kap...@huawei.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. pg_basebkup_recvxlog_dump_conn_str_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 From ebfcff54ae934b38f3bfecfbe8dbe0cbe0573c95 Mon Sep 17 00:00:00 2001 From: Heikki Linnakangas heikki.linnakan...@iki.fi Date: Wed, 20 Feb 2013 20:49:24 +0200 Subject: [PATCH 1/3] Add -d option, for specifying a connection string, to pg_basebackup and pg_receivexlog. It's a bit strange that the option is called -d/--dbname, when in fact you can *not* pass a database name to it. But it's consistent with other client tools, where you can pass a connection string using the -d option. --- doc/src/sgml/ref/pg_basebackup.sgml| 12 + doc/src/sgml/ref/pg_receivexlog.sgml | 12 + src/bin/pg_basebackup/pg_basebackup.c |7 ++- src/bin/pg_basebackup/pg_receivexlog.c |7 ++- src/bin/pg_basebackup/streamutil.c | 87 +++- src/bin/pg_basebackup/streamutil.h |1 + 6 files changed, 101 insertions(+), 25 deletions(-) diff --git a/doc/src/sgml/ref/pg_basebackup.sgml b/doc/src/sgml/ref/pg_basebackup.sgml index 2f89f2c..3ab460a 100644 --- a/doc/src/sgml/ref/pg_basebackup.sgml +++ b/doc/src/sgml/ref/pg_basebackup.sgml @@ -359,6 +359,18 @@ PostgreSQL documentation variablelist varlistentry + termoption-d replaceable class=parameterconnstr/replaceable/option/term + termoption--dbname=replaceable class=parameterconnstr/replaceable/option/term + listitem + para +Specifies connection string options, used for connecting to server. +These options can be used along with other user supplied options. +In case of aconflicting options, the user supplied option is used. + /para + /listitem + /varlistentry + + varlistentry termoption-h replaceable class=parameterhost/replaceable/option/term termoption--host=replaceable class=parameterhost/replaceable/option/term listitem diff --git a/doc/src/sgml/ref/pg_receivexlog.sgml b/doc/src/sgml/ref/pg_receivexlog.sgml index d06dd1f..314da0e 100644 --- a/doc/src/sgml/ref/pg_receivexlog.sgml +++ b/doc/src/sgml/ref/pg_receivexlog.sgml @@ -123,6 +123,18 @@ PostgreSQL documentation variablelist varlistentry + termoption-d replaceable class=parameterconnstr/replaceable/option/term + termoption--dbname=replaceable class=parameterconnstr/replaceable/option/term + listitem + para +Specifies connection string options, used for connecting to server. +These options can be used along with
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Wed, Feb 20, 2013 at 2:16 PM, Heikki Linnakangas hlinnakan...@vmware.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
-Original Message- From: pgsql-hackers-ow...@postgresql.org [mailto:pgsql-hackers- ow...@postgresql.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 kapilaamit.kap...@huawei.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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 kapilaamit.kap...@huawei.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 kapilaamit.kap...@huawei.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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
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 kapilaamit.kap...@huawei.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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila amit.kap...@huawei.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 alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.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$@ko...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Sunday, February 17, 2013 8:44 PM Phil Sorber wrote: On Sun, Feb 17, 2013 at 1:35 AM, Amit kapila amit.kap...@huawei.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 alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.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$@ko...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
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 alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.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$@ko...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On 04.02.2013 17:32, Alvaro Herrera wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.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$@ko...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Mon, Feb 11, 2013 at 4:19 PM, Heikki Linnakangas hlinnakan...@vmware.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 alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herreraalvhe...@2ndquadrant.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$@ko...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Feb 3, 2013 4:16 AM, Phil Sorber p...@omniti.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: On Feb 3, 2013 4:16 AM, Phil Sorber p...@omniti.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
Phil Sorber wrote: On Mon, Feb 4, 2013 at 10:16 AM, Alvaro Herrera alvhe...@2ndquadrant.com wrote: Phil Sorber wrote: On Mon, Feb 4, 2013 at 9:13 AM, Alvaro Herrera alvhe...@2ndquadrant.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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Feb 3, 2013 4:16 AM, Phil Sorber p...@omniti.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
Re: [HACKERS] [PATCH] Add PQconninfoParseParams and PQconninfodefaultsMerge to libpq
On Sun, Feb 3, 2013 at 1:37 AM, Magnus Hagander mag...@hagander.net wrote: On Feb 3, 2013 4:16 AM, Phil Sorber p...@omniti.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... libpq_params_parse_merge.diff Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
