Re: Add --tablespace option to reindexdb

> On 26 Feb 2021, at 07:49, Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> Since c5b2860, it is possible to specify a tablespace for a REINDEX,
> but the equivalent option has not been added to reindexdb. Attached
> is a patch to take care of that.
>
> This includes documentation and tests.

Makes sense.

> While on it, I have added tests for toast tables and indexes with a
> tablespace move during a REINDEX. Those operations fail, but it is
> not possible to get that into the main regression test suite because
> the error messages include the relation names so that's unstable.
> Well, it would be possible to do that for the non-concurrent case
> using a TRY/CATCH block in a custom function but that would not work
> with CONCURRENTLY. Anyway, I would rather group the whole set of
> tests together, and using the --tablespace option introduced here
> within a TAP test does the job.

Agreed, doing it with a TAP test removes the complication.

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()?

+ 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)?

--
Daniel Gustafsson https://vmware.com/