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