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

  1. Add --tablespace option to reindexdb

  2. Fix duplicated test case in TAP tests of reindexdb