Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly

Hi Michael,

Thank you for your comments.

On 19.09.2019 7:43, Michael Paquier wrote:
> On Wed, Sep 18, 2019 at 03:46:20PM +0300, Alexey Kondratov wrote:
>> Currently in Postgres SET TABLESPACE always comes with [ NOWAIT ] option, so
>> I hope it worth adding this option here for convenience. Added in the new
>> version.
> It seems to me that it would be good to keep the patch as simple as
> possible for its first version, and split it into two if you would
> like to add this new option instead of bundling both together. This
> makes the review of one and the other more simple.

OK, it makes sense. I would also prefer first patch as simple as
possible, but adding this NOWAIT option required only a few dozens of
lines, so I just bundled everything together. Anyway, I will split
patches if we decide to keep [ SET TABLESPACE ... [NOWAIT] ] grammar.

> Anyway, regarding
> the grammar, is SET TABLESPACE really our best choice here? What
> about:
> - TABLESPACE = foo, in parenthesis only?
> - Only using TABLESPACE, without SET at the end of the query?
>
> SET is used in ALTER TABLE per the set of subqueries available there,
> but that's not the case of REINDEX.

I like SET TABLESPACE grammar, because it already exists and used both
in ALTER TABLE and ALTER INDEX. Thus, if we once add 'ALTER INDEX
index_name REINDEX SET TABLESPACE' (as was proposed earlier in the
thread), then it will be consistent with 'REINDEX index_name SET
TABLESPACE'. If we use just plain TABLESPACE, then it may be misleading
in the following cases:

- REINDEX TABLE table_name TABLESPACE tablespace_name
- REINDEX (TABLESPACE = tablespace_name) TABLE table_name

since it may mean 'Reindex all indexes of table_name, that stored in the
tablespace_name', doesn't it?

However, I have rather limited experience with Postgres, so I doesn't
insist.

> +-- check that all relations moved to new tablespace
> +SELECT relname FROM pg_class
> +WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE
> spcname='regress_tblspace')
> +AND relname IN ('regress_tblspace_test_tbl_idx');
> + relname
> +-------------------------------
> + regress_tblspace_test_tbl_idx
> +(1 row)
> Just to check one relation you could use \d with the relation (index
> or table) name.

Yes, \d outputs tablespace name if it differs from pg_default, but it
shows other information in addition, which is not necessary here. Also
its output has more chances to be changed later, which may lead to the
failed tests. This query output is more or less stable and new relations
may be easily added to tests if we once add tablespace change to
CLUSTER/VACUUM FULL. I can change test to use \d, but not sure that it
would reduce test output length or will be helpful for a future tests
support.

> - if (RELATION_IS_OTHER_TEMP(iRel))
> - ereport(ERROR,
> - (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> - errmsg("cannot reindex temporary tables of other
> - sessions")))
> I would keep the order of this operation in order with
> CheckTableNotInUse().

Sure, I haven't noticed that reordered these operations, thanks.

--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company