Re: Add --tablespace option to reindexdb
Michael Paquier <michael@paquier.xyz>
From: Michael Paquier <michael@paquier.xyz>
To: Daniel Gustafsson <daniel@yesql.se>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Date: 2021-02-26T11:14:56Z
Lists: pgsql-hackers
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
Commits
-
Add --tablespace option to reindexdb
- 57e6db706e81 14.0 landed
-
Fix duplicated test case in TAP tests of reindexdb
- 0f1b0c0b60ef 12.7 landed
- 621e8a603d56 13.3 landed
- c5530d8474a4 14.0 landed