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

On 2021-01-22 00:26, Justin Pryzby wrote:
> 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 ?
>

I think so. I would prefer to do not move TOAST indexes implicitly at
all during reindex.

>
>> @@ -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.

Sounds good to me.

I have updated patches accordingly and also simplified tablespaceOid
checks and assignment in the newly added SetRelTableSpace(). Result is
attached as two separate patches for an ease of review, but no
objections to merge them and apply at once if everything is fine.

Regards
--
Alexey Kondratov

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