Hi Laurenz, Thank you for taking the time to review that patch.
Le lun. 25 nov. 2019 à 22:34, Laurenz Albe <laurenz.a...@cybertec.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.*
