Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>,
 Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
 Steve Singer <steve@ssinger.info>, pgsql-hackers@lists.postgresql.org,
 Alvaro Herrera <alvherre@2ndquadrant.com>,
 Robert Haas <robertmhaas@gmail.com>,
 Alexander Korotkov <a.korotkov@postgrespro.ru>,
 Masahiko Sawada <sawada.mshk@gmail.com>,
 Jose Luis Tallon <jltallon@adv-solutions.net>
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>
From: Alexey Kondratov <a.kondratov@postgrespro.ru>
Message-ID: <cb874a90-3946-91e4-4c31-5075f425b280@postgrespro.ru>
Date: Thu, 12 Mar 2020 20:08:46 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <20200309200447.GA32459@telsasoft.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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