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

On 2021-01-28 14:42, Alexey Kondratov wrote:
> On 2021-01-28 00:36, Alvaro Herrera wrote:

>> I didn't look at the patch closely enough to understand why you're
>> trying to do something like CLUSTER, VACUUM FULL or REINDEX without
>> holding full AccessExclusiveLock on the relation. But do keep in mind
>> that once you hold a lock on a relation, trying to grab a weaker lock
>> afterwards is pretty pointless.
>>
>
> No, you are right, we are doing REINDEX with AccessExclusiveLock as it
> was before. This part is a more specific one. It only applies to
> partitioned indexes, which do not hold any data, so we do not reindex
> them directly, only their leafs. However, if we are doing a TABLESPACE
> change, we have to record it in their pg_class entry, so all future
> leaf partitions were created in the proper tablespace.
>
> That way, we open partitioned index relation only for a reference,
> i.e. read-only, but modify pg_class entry under a proper lock
> (RowExclusiveLock). That's why I thought that ShareLock will be
> enough.
>
> IIUC, 'ALTER TABLE ... SET TABLESPACE' uses AccessExclusiveLock even
> for relations with no storage, since AlterTableGetLockLevel() chooses
> it if AT_SetTableSpace is met. This is very similar to our case, so
> probably we should do the same?
>
> Actually it is not completely clear for me why
> ShareUpdateExclusiveLock is sufficient for newly added
> SetRelationTableSpace() as Michael wrote in the comment.
>

Changed patch to use AccessExclusiveLock in this part for now. This is
what 'ALTER TABLE/INDEX ... SET TABLESPACE' and 'REINDEX' usually do.
Anyway, all real leaf partitions are processed in the independent
transactions later.

Also changed some doc/comment parts Justin pointed me to.

>> + then all "mapped" and system relations will be skipped and a
>> single
>> + <literal>WARNING</literal> will be generated. Indexes on TOAST
>> tables
>> + are reindexed, but not moved the new tablespace.
>
> moved *to* the new tablespace.
>

Fixed.

>
> I don't know if that needs to be said at all. We talked about it a lot
> to
> arrive at the current behavior, but I think that's only due to the
> difficulty
> of correcting the initial mistake.
>

I do not think that it will be a big deal to move indexes on TOAST
tables as well. I just thought that since 'ALTER TABLE/INDEX ... SET
TABLESPACE' only moves them together with host table, we also should not
do that. Yet, I am ready to change this logic if requested.

Regards
--
Alexey Kondratov

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