Re: [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