agora inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Justin Pryzby <pryzby@telsasoft.com>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>
Cc: Alexander Korotkov <a.korotkov@postgrespro.ru>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: Wed, 03 Feb 2021 13:35:26 +0300
Message-ID: <66bad38beb87fb32952d89602e467444@postgrespro.ru> (raw)
In-Reply-To: <YBpEsxZxIVn0zyFM@paquier.xyz>
References: <20210127213658.GA736@alvherre.pgsql>
<55c6c1526b94d2caf54c33262fe00674@postgrespro.ru>
<e15777decc533006c9dbe4988bd66d31@postgrespro.ru>
<YBTC0IYIGmyYfiIn@paquier.xyz>
<1c506564e2a732ec17790bd8c34f042e@postgrespro.ru>
<YBiro+n0767QDXSA@paquier.xyz>
<YBpEsxZxIVn0zyFM@paquier.xyz>
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
view thread (154+ messages) latest in thread
Message-ID: <66bad38beb87fb32952d89602e467444@postgrespro.ru>
Permalink: ../66bad38beb87fb32952d89602e467444@postgrespro.ru/
Also on: postgresql.org/message-id/66bad38beb87fb32952d89602e467444@postgrespro.ru
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: a.kondratov@postgrespro.ru, michael@paquier.xyz, alvherre@alvh.no-ip.org, pryzby@telsasoft.com, peter.eisentraut@enterprisedb.com, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, robertmhaas@gmail.com, a.korotkov@postgrespro.ru, sawada.mshk@gmail.com, jltallon@adv-solutions.net
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
In-Reply-To: <66bad38beb87fb32952d89602e467444@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox