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

On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote:
> I have updated patches accordingly and also simplified tablespaceOid checks
> and assignment in the newly added SetRelTableSpace(). Result is attached as
> two separate patches for an ease of review, but no objections to merge them
> and apply at once if everything is fine.

extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass);
+extern bool SetRelTableSpace(Oid reloid, Oid tablespaceOid);
Seeing SetRelationHasSubclass(), wouldn't it be more consistent to use
SetRelationTableSpace() as routine name?

I think that we should document that the caller of this routine had
better do a CCI once done to make the tablespace chage visible.
Except for those two nits, the patch needs an indentation run and some
style tweaks but its logic looks fine. So I'll apply that first
piece.

+SELECT relname FROM pg_class
+WHERE reltablespace=(SELECT oid FROM pg_tablespace WHERE spcname='regress_tblspace');
[...]
+-- first, check a no-op case
+REINDEX (TABLESPACE pg_default) INDEX regress_tblspace_test_tbl_idx;
+REINDEX (TABLESPACE pg_default) TABLE regress_tblspace_test_tbl;
Reindexing means that the relfilenodes are changed, so the tests
should track the original and new relfilenodes and compare them, no?
In short, this set of regression tests does not make sure that a
REINDEX actually happens or not, and this may read as a reindex not
happening at all for those tests. For single units, these could be
saved in a variable and compared afterwards. create_index.sql does
that a bit with REINDEX SCHEMA for a set of relations.

+INSERT INTO regress_tblspace_test_tbl (num1, num2, t)
+ SELECT round(random()*100), random(), repeat('text', 1000000)
+ FROM generate_series(1, 10) s(i);
Repeating 1M times a text value is too costly for such a test. And as
even for empty tables there is one page created for toast indexes,
there is no need for that?

This patch is introducing three new checks for system catalogs:
- don't use tablespace for mapped relations.
- don't use tablespace for system relations, except if
allowSystemTableMods.
- don't move non-shared relation to global tablespace.
For the non-concurrent case, all three checks are in reindex_index().
For the concurrent case, the two first checks are in
ReindexMultipleTables() and the third one is in
ReindexRelationConcurrently(). That's rather tricky to follow because
CONCURRENTLY is not allowed on system relations. I am wondering if it
would be worth an extra comment effort, or if there is a way to
consolidate that better.

typedef struct ReindexParams
{
bits32 options; /* bitmask of REINDEXOPT_* */
+ Oid tablespaceOid; /* tablespace to rebuild index */
} ReindexParams;
For DDL commands, InvalidOid on a tablespace means to usually use the
system's default. However, for REINDEX, it means that the same
tablespace as the origin would be used. I think that this had better
be properly documented in this structure.

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

+ * "tablespaceOid" is the new tablespace to use for this index.
+ * If InvalidOid, use the current tablespace.
[...]
+ * See comments of reindex_relation() for details about "tablespaceOid".
Those comments are wrong as the tablespace OID is not part of
ReindexParams.

There is no documentation about the behavior of toast tables with
TABLESPACE. In this case, the patch mentions that the option will not
work directly on system catalogs unless allow_system_table_mods is
true, but it forgets to tell that it does not move toast indexes,
still these are getting reindexed.

There are no regression tests stressing the tablespace ACL check for
the concurrent *and* the non-concurrent cases.

There is one ACL check in ReindexPartitions(), and a second one in
reindex_index(), but it seems to me that you are missing the path for
concurrent indexes. It would be tempting to have the check directly
in ExecReindex() to look after everything at the earliest stage
possible, but we still need to worry about the multi-transaction case.
--
Michael