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

On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:
> Attached is a new patch set of first two patches, that should resolve all
> the issues raised before (ACL, docs, tests) excepting TOAST. Double thanks
> for suggestion to add more tests with nested partitioning. I have found and
> squashed a huge bug related to the returning back to the default tablespace
> using newly added tests.
> 
> Regarding TOAST. Now we skip moving toast indexes or throw error if someone
> wants to move TOAST index directly. I had a look on ALTER TABLE SET
> TABLESPACE and it has a bit complicated logic:
> 
> 1) You cannot move TOAST table directly.
> 2) But if you move basic relation that TOAST table belongs to, then they are
> moved altogether.
> 3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE ...
> 
> That way, ALTER TABLE allows moving TOAST tables (with indexes) implicitly,
> but does not allow doing that explicitly. In the same time I found docs to
> be vague about such behavior it only says:
> 
>     All tables in the current database in a tablespace can be moved
>     by using the ALL IN TABLESPACE ... Note that system catalogs are
>     not moved by this command
> 
>     Changing any part of a system catalog table is not permitted.
> 
> So actually ALTER TABLE treats TOAST relations as system sometimes, but
> sometimes not.
> 
> From the end user perspective it makes sense to move TOAST with main table
> when doing ALTER TABLE SET TABLESPACE. But should we touch indexes on TOAST
> table with REINDEX? We cannot move TOAST relation itself, since we are doing
> only a reindex, so we end up in the state when TOAST table and its index are
> placed in the different tablespaces. This state is not reachable with ALTER
> TABLE/INDEX, so it seem we should not allow it with REINDEX as well, should
> we?

> +		 * Even if a table's indexes were moved to a new tablespace, the index
> +		 * on its toast table is not normally moved.
>  		 */
>  		ReindexParams newparams = *params;
>  
>  		newparams.options &= ~(REINDEXOPT_MISSING_OK);
> +		if (!allowSystemTableMods)
> +			newparams.tablespaceOid = InvalidOid;

I think you're right.  So actually TOAST should never move, even if
allowSystemTableMods, right ?

> @@ -292,7 +315,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
>     with <command>REINDEX INDEX</command> or <command>REINDEX TABLE</command>,
>     respectively. Each partition of the specified partitioned relation is
>     reindexed in a separate transaction. Those commands cannot be used inside
> -   a transaction block when working on a partitioned table or index.
> +   a transaction block when working on a partitioned table or index. If
> +   <command>REINDEX</command> with <literal>TABLESPACE</literal> executed
> +   on partitioned relation fails it may have moved some partitions to the new
> +   tablespace. Repeated command will still reindex all partitions even if they
> +   are already in the new tablespace.

Minor corrections here:

If a <command>REINDEX</command> command fails when run on a partitioned
relation, and <literal>TABLESPACE</literal> was specified, then it may have
moved indexes on some partitions to the new tablespace.  Re-running the command
will reindex all partitions and move previously-unprocessed indexes to the new
tablespace.


-- 
Justin