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

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.

- 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.

- There are two places you forgot:

createdb --maintenance-db=dbname
dropdb --maintenance-db=dbname

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.

Yours,
Laurenz Albe