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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Justin Pryzby <pryzby(at)telsasoft(dot)com>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: 2021-02-03 10:35:26
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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,
> - errmsg("cannot reindex temporary tables of other
> sessions")));
> + 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
+-- 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
+ ORDER BY relid, level;
+SELECT relid, parentrelid, level FROM
+ 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.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2021-02-03 10:52:26 Re: [PATCH] postgres_fdw connection caching - cause remote sessions linger till the local session exit
Previous Message Amit Kapila 2021-02-03 10:26:30 Re: POC: Cleaning up orphaned files using undo logs