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