Re: [Doc] pg_restore documentation didn't explain how to use connection string

Hi Laurenz,

Thank you for taking the time to review that patch.

Le lun. 25 nov. 2019 à 22:34, Laurenz Albe <laurenz(dot)albe(at)cybertec(dot)at> a
écrit :

> On Wed, 2019-11-13 at 16:48 +0100, Lætitia Avrot wrote:
> > So after some thoughts I did the minimal patch (for now).
> > I corrected documentation for the following tools so that now, using
> connection string
> > for Postgres client applications is documented in Postgres:
> > - clusterdb
> > - pgbench
> > - pg_dump
> > - pg_restore
> > - reindexdb
> > - vacuumdb
>
> I think that this patch is a good idea.
> Even if it adds some redundancy, that can hardly be avoided because, as
> you said,
> the options to specify the database name are not the same everywhere.
>
> The patch applies and build fine.
>
> I see some room for improvement:
>
> - I think that "connection string" is better than "conninfo string".
> At least the chapter to which you link is headed "Connection Strings".
>
> This would also be consistent with the use of that term in the
> "pg_basebackup" , "pg_dumpall" and "pg_receivewal" documentation.
>
> You seem to have copied that wording from the "pg_isready", "psql",
> "reindexdb" and "vacuumdb" documentation, but I think it would be better
> to reword those too.
>
> I agree.

> - You begin your paragraph with "if this parameter contains ...".
>
> First, I think "argument" might be more appropriate here, as you
> are talking about
> a) the supplied value and
> b) a command line argument or the argument to an option
>
> Besides, it might be confusing to refer to "*this* parameter" if the text
> is not immediately after what you are referring to, like for example
> in "pgbench", where it might refer to the -h, -p or -U options.
>
> I think it would be better and less ambiguous to use
> "If <replaceable class="parameter">dbname</replaceable> contains ..."
>
> In the cases where there is no ambiguity, it might be better to use
> a wording like in the "pg_recvlogical" documentation.
>
> You're right.

> - There are two places you forgot:
>
> createdb --maintenance-db=dbname
> dropdb --maintenance-db=dbname
>
> You're perfectly right!

> While looking at this patch, I noticed that "createuser" and "dropuser"
> explicitly connect to the "postgres" database rather than using
> "connectMaintenanceDatabase()" like the other scripts, which would try
> the database "postgres" first and fall back to "template1".
>
> This is unrelated to the patch, but low-hanging fruit for unified behavior.
>

I agree and while trying to unify everything, you'r better try and make
right for all the tools.

I'm not very satisfied with this patch. I think I want to go further with
unifying connection string usage. I'd like at least each and every client
app to accept the same syntax and argument. Let me think a little further
on it, so I try to come up with a simple and neat solution.

Several ones are possible and I'd like to find them all to be able to pick
the best.

Have a nice day,

Lætitia
--
*Paper doesn’t grow on trees. Please print responsibly.*