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
signature.asc
Description: PGP signature