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