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

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