From: Alexey Kondratov <a.kondratov@postgrespro.ru>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Masahiko Sawada <masahiko.sawada@2ndquadrant.com>
Cc: Steve Singer <steve@ssinger.info>
Cc: pgsql-hackers@lists.postgresql.org, Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: 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: Thu, 12 Mar 2020 20:08:46 +0300
Message-ID: <cb874a90-3946-91e4-4c31-5075f425b280@postgrespro.ru> (raw)
In-Reply-To: <20200309200447.GA32459@telsasoft.com>
References: <157395200750.29912.1178609357962324139.pgcf@coridan.postgresql.org>
<827a9139-e02b-2dbf-5c6c-a6fbcaa1739a@postgrespro.ru>
<CA+fd4k4uO5HS8Rb0PgH5oWQYT9xWYONZmLGUyAGvANFke-8c3Q@mail.gmail.com>
<20191127035416.GG5435@paquier.xyz>
<3ae48673-283c-3e99-3dd8-36ebb81614b5@postgrespro.ru>
<20191202082134.GI1696@paquier.xyz>
<c8ed43871e171ca2077aa9e1d0ed970a@postgrespro.ru>
<20200211164848.GO1412@telsasoft.com>
<eb4cdddc0d6197f3fef15d36758c93fe@postgrespro.ru>
<20200229145304.GI29456@telsasoft.com>
<20200309200447.GA32459@telsasoft.com>
Hi Justin,
On 09.03.2020 23:04, Justin Pryzby wrote:
> On Sat, Feb 29, 2020 at 08:53:04AM -0600, Justin Pryzby wrote:
>> On Sat, Feb 29, 2020 at 03:35:27PM +0300, Alexey Kondratov wrote:
>>> Anyway, new version is attached. It is rebased in order to resolve conflicts
>>> with a recent fix of REINDEX CONCURRENTLY + temp relations, and includes
>>> this small comment fix.
>> Thanks for rebasing - I actually started to do that yesterday.
>>
>> I extracted the bits from your original 0001 patch which handled CLUSTER and
>> VACUUM FULL. I don't think if there's any interest in combining that with
>> ALTER anymore. On another thread (1), I tried to implement that, and Tom
>> pointed out problem with the implementation, but also didn't like the idea.
>>
>> I'm including some proposed fixes, but didn't yet update the docs, errors or
>> tests for that. (I'm including your v8 untouched in hopes of not messing up
>> the cfbot). My fixes avoid an issue if you try to REINDEX onto pg_default, I
>> think due to moving system toast indexes.
> I was able to avoid this issue by adding a call to GetNewRelFileNode, even
> though that's already called by RelationSetNewRelfilenode(). Not sure if
> there's a better way, or if it's worth Alexey's v3 patch which added a
> tablespace param to RelationSetNewRelfilenode.
Do you have any understanding of what exactly causes this error? I have
tried to debug it a little bit, but still cannot figure out why we need
this extra GetNewRelFileNode() call and a mechanism how it helps.
Probably you mean v4 patch. Yes, interestingly, if we do everything at
once inside RelationSetNewRelfilenode(), then there is no issue at all with:
REINDEX DATABASE template1 TABLESPACE pg_default;
It feels like I am doing a monkey coding here, so I want to understand
it better :)
> The current logic allows moving all the indexes and toast indexes, but I think
> we should use IsSystemRelation() unless allow_system_table_mods, like existing
> behavior of ALTER.
>
> template1=# ALTER TABLE pg_extension_oid_index SET tablespace pg_default;
> ERROR: permission denied: "pg_extension_oid_index" is a system catalog
> template1=# REINDEX INDEX pg_extension_oid_index TABLESPACE pg_default;
> REINDEX
Yeah, we definitely should obey the same rules as ALTER TABLE / INDEX in
my opinion.
> Finally, I think the CLUSTER is missing permission checks. It looks like
> relation_is_movable was factored out, but I don't see how that helps ?
I did this relation_is_movable refactoring in order to share the same
check between REINDEX + TABLESPACE and ALTER INDEX + SET TABLESPACE.
Then I realized that REINDEX already has its own temp tables check and
does mapped relations validation in multiple places, so I just added
global tablespace checks instead. Thus, relation_is_movable seems to be
outdated right now. Probably, we have to do another refactoring here
once all proper validations will be accumulated in this patch set.
> Alexey, I'm hoping to hear back if you think these changes are ok or if you'll
> publish a new version of the patch addressing the crash I reported.
> Or if you're too busy, maybe someone else can adopt the patch (I can help).
Sorry for the late response, I was not going to abandon this patch, but
was a bit busy last month.
Many thanks for you review and fixups! There are some inconsistencies
like mentions of SET TABLESPACE in error messages and so on. I am going
to refactor and include your fixes 0003-0004 into 0001 and 0002, but
keep 0005 separated for now, since this part requires more understanding
IMO (and comparison with v4 implementation).
That way, I am going to prepare a more clear patch set till the middle
of the next week. I will be glad to receive more feedback from you then.
Regards
--
Alexey Kondratov
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
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, pryzby@telsasoft.com, michael@paquier.xyz, masahiko.sawada@2ndquadrant.com, steve@ssinger.info, alvherre@2ndquadrant.com, 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: <cb874a90-3946-91e4-4c31-5075f425b280@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 DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox