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

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.

+REINDEX (TABLESPACE regress_tblspace) SYSTEM CONCURRENTLY postgres; -- fail
+ERROR: cannot reindex system catalogs concurrently
[...]
+REINDEX (TABLESPACE regress_tblspace) DATABASE regression; -- ok with warning
+WARNING: cannot change tablespace of indexes on system relations, skipping all
+REINDEX (TABLESPACE pg_default) DATABASE regression; -- ok with warning
+WARNING: cannot change tablespace of indexes on system relations, skipping all
Those tests are costly by design, so let's drop them. They have been
useful to check the patch, but if tests are changed with objects
remaining around this would cost a lot of resources.

+ /* 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 totablespace \"%s\"",
+ get_tablespace_name(params->tablespaceOid))));
There is no test coverage for this case with REINDEX CONCURRENTLY, and
that's easy enough to stress. So I have added one.

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.
--
Michael