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

On 2021-02-03 09:37, Michael Paquier wrote:
> On Tue, Feb 02, 2021 at 10:32:19AM +0900, Michael Paquier wrote:
>> On Mon, Feb 01, 2021 at 06:28:57PM +0300, Alexey Kondratov wrote:
>> > Hm, IIUC, REINDEX CONCURRENTLY doesn't use either of them. It directly uses
>> > index_create() with a proper tablespaceOid instead of
>> > SetRelationTableSpace(). And its checks structure is more restrictive even
>> > without tablespace change, so it doesn't use CheckRelationTableSpaceMove().
>> 
>> Sure.  I have not checked the patch in details, but even with that it
>> would be much safer to me if we apply the same sanity checks
>> everywhere.  That's less potential holes to worry about.
> 
> Thanks Alexey for the new patch.  I have been looking at the main
> patch in details.
> 
>     /*
> -    * Don't allow reindex on temp tables of other backends ... their 
> local
> -    * buffer manager is not going to cope.
> +    * We don't support moving system relations into different 
> tablespaces
> +    * unless allow_system_table_mods=1.
>      */
> If you remove the check on RELATION_IS_OTHER_TEMP() in
> reindex_index(), you would allow the reindex of a temp relation owned
> by a different session if its tablespace is not changed, so this
> cannot be removed.
> 
> +        !allowSystemTableMods && IsSystemRelation(iRel))
>          ereport(ERROR,
> -                (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> -                 errmsg("cannot reindex temporary tables of other 
> sessions")));
> +                (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> +                 errmsg("permission denied: \"%s\" is a system 
> catalog",
> +                        RelationGetRelationName(iRel))));
> Indeed, a system relation with a relfilenode should be allowed to move
> under allow_system_table_mods.  I think that we had better move this
> check into CheckRelationTableSpaceMove() instead of reindex_index() to
> centralize the logic.  ALTER TABLE does this business in
> RangeVarCallbackForAlterRelation(), but our code path opening the
> relation is different for the non-concurrent case.
> 
> +       if (OidIsValid(params->tablespaceOid) &&
> +           IsSystemClass(relid, classtuple))
> +       {
> +           if (!allowSystemTableMods)
> +           {
> +               /* Skip all system relations, if not 
> allowSystemTableMods *
> I don't see the need for having two warnings here to say the same
> thing if a relation is mapped or not mapped, so let's keep it simple.
> 

Yeah, I just wanted to separate mapped and system relations, but 
probably it is too complicated.

> 
> I have found that the test suite was rather messy in its
> organization.  Table creations were done first with a set of tests not
> really ordered, so that was really hard to follow.  This has also led
> to a set of tests that were duplicated, while other tests have been
> missed, mainly some cross checks for the concurrent and non-concurrent
> behaviors.  I have reordered the whole so as tests on catalogs, normal
> tables and partitions are done separately with relations created and
> dropped for each set.  Partitions use a global check for tablespaces
> and relfilenodes after one concurrent reindex (didn't see the point in
> doubling with the non-concurrent case as the same code path to select
> the relations from the partition tree is taken).  An ACL test has been
> added at the end.
> 
> The case of partitioned indexes was kind of interesting and I thought
> about that a couple of days, and I took the decision to ignore
> relations that have no storage as you did, documenting that ALTER
> TABLE can be used to update the references of the partitioned
> relations.  The command is still useful with this behavior, and the
> tests I have added track that.
> 
> Finally, I have reworked the docs, separating the limitations related
> to system catalogs and partitioned relations, to be more consistent
> with the notes at the end of the page.
> 

Thanks for working on this.

+	if (tablespacename != NULL)
+	{
+		params.tablespaceOid = get_tablespace_oid(tablespacename, false);
+
+		/* Check permissions except when moving to database's default */
+		if (OidIsValid(params.tablespaceOid) &&

This check for OidIsValid() seems to be excessive, since you moved the 
whole ACL check under 'if (tablespacename != NULL)' here.

+			params.tablespaceOid != MyDatabaseTableSpace)
+		{
+			AclResult	aclresult;


+CREATE INDEX regress_tblspace_test_tbl_idx ON regress_tblspace_test_tbl 
(num1);
+-- move to global tablespace move fails

Maybe 'move to global tablespace, fail', just to match a style of the 
previous comments.

+REINDEX (TABLESPACE pg_global) INDEX regress_tblspace_test_tbl_idx;


+SELECT relid, parentrelid, level FROM 
pg_partition_tree('tbspace_reindex_part_index')
+  ORDER BY relid, level;
+SELECT relid, parentrelid, level FROM 
pg_partition_tree('tbspace_reindex_part_index')
+  ORDER BY relid, level;

Why do you do the same twice in a row? It looks like a typo. Maybe it 
was intended to be called for partitioned table AND index.


Regards
-- 
Alexey Kondratov

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