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