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