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
