On Fri, Feb 26, 2021 at 11:00:13AM +0100, Daniel Gustafsson wrote: > Some other small comments: > > + Assert(PQserverVersion(conn) >= 140000); > Are these assertions really buying us much when we already check the server > version in reindex_one_database()?
I found these helpful when working on vacuumdb and refactoring the
code, though I'd agree this code may not justify going down to that.
> + printf(_(" --tablespace=TABLESPACE reindex on specified
> tablespace\n"));
> While not introduced by this patch, I realized that we have a mix of
> conventions for how to indent long options which don't have a short option.
> Under "Connection options" all options are left-justified but under "Options"
> all long-options are aligned with space indentation for missing shorts. Some
> tools do it like this, where others like createuser left justifies under
> Options as well. Maybe not the most pressing issue, but consistency is always
> good in user interfaces so maybe it's worth addressing (in a separate patch)?
Yeah, consistency is good, though I am not sure which one would be
consistent here actually as there is no defined rule. For this one,
I think that I would keep what I have to be consistent with the
existing inconsistency (?). In short, I would just add --tablespace
the same way there is --concurrently, without touching the connection
option part.
--
Michael
signature.asc
Description: PGP signature
