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

On 2021-01-27 06:14, Michael Paquier wrote:
> On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
>> In the new 0002 I moved ACL check to the upper level, i.e. 
>> ExecReindex(),
>> and removed expensive text generation in test. Not touched yet some of 
>> your
>> previously raised concerns. Also, you made SetRelationTableSpace() to 
>> accept
>> Relation instead of Oid, so now we have to open/close indexes in the
>> ReindexPartitions(), I am not sure that I use proper locking there, 
>> but it
>> works.
> 
> Passing down Relation to the new routines makes the most sense to me
> because we force the callers to think about the level of locking
> that's required when doing any tablespace moves.
> 
> +           Relation iRel = index_open(partoid, ShareLock);
> +
> +           if (CheckRelationTableSpaceMove(iRel, 
> params->tablespaceOid))
> +               SetRelationTableSpace(iRel,
> +                                     params->tablespaceOid,
> +                                     InvalidOid);
> Speaking of which, this breaks the locking assumptions of
> SetRelationTableSpace().  I feel that we should think harder about
> this part for partitioned indexes and tables because this looks rather
> unsafe in terms of locking assumptions with partition trees.  If we
> cannot come up with a safe solution, I would be fine with disallowing
> TABLESPACE in this case, as a first step.  Not all problems have to be
> solved at once, and even without this part the feature is still
> useful.
> 

I have read more about lock levels and ShareLock should prevent any kind 
of physical modification of indexes. We already hold ShareLock doing 
find_all_inheritors(), which is higher than ShareUpdateExclusiveLock, so 
using ShareLock seems to be safe here, but I will look on it closer.

> 
> +   /* It's not a shared catalog, so refuse to move it to shared 
> tablespace */
> +   if (params->tablespaceOid == GLOBALTABLESPACE_OID)
> +       ereport(ERROR,
> +               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> +                errmsg("cannot move non-shared relation to tablespace 
> \"%s\"",
> +                    get_tablespace_name(params->tablespaceOid))));
> Why is that needed if CheckRelationTableSpaceMove() is used?
> 

This is from ReindexRelationConcurrently() where we do not use 
CheckRelationTableSpaceMove(). For me it makes sense to add only this 
GLOBALTABLESPACE_OID check there, since before we already check for 
system catalogs and after for temp relations, so adding 
CheckRelationTableSpaceMove() will be a double-check.

> 
> -                             indexRelation->rd_rel->reltablespace,
> +                             OidIsValid(tablespaceOid) ?
> +                               tablespaceOid :
> indexRelation->rd_rel->reltablespace,
> Let's remove this logic from index_concurrently_create_copy() and let
> the caller directly decide the tablespace to use, without a dependency
> on InvalidOid in the inner routine.  A share update exclusive lock is
> already hold on the old index when creating the concurrent copy, so
> there won't be concurrent schema changes.
> 

Changed.

Also added tests for ACL checks, relfilenode changes. Added ACL recheck 
for multi-transactional case. Added info about TOAST index reindexing. 
Changed some comments.


Regards
-- 
Alexey Kondratov

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