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

On 2021-01-25 11:07, Michael Paquier wrote:
> On Fri, Jan 22, 2021 at 05:07:02PM +0300, Alexey Kondratov wrote:
>> I have updated patches accordingly and also simplified tablespaceOid 
>> checks
>> and assignment in the newly added SetRelTableSpace(). Result is 
>> attached as
>> two separate patches for an ease of review, but no objections to merge 
>> them
>> and apply at once if everything is fine.
> 
>  extern void SetRelationHasSubclass(Oid relationId, bool 
> relhassubclass);
> +extern bool SetRelTableSpace(Oid reloid, Oid tablespaceOid);
> Seeing SetRelationHasSubclass(), wouldn't it be more consistent to use
> SetRelationTableSpace() as routine name?
> 
> I think that we should document that the caller of this routine had
> better do a CCI once done to make the tablespace chage visible.
> Except for those two nits, the patch needs an indentation run and some
> style tweaks but its logic looks fine.  So I'll apply that first
> piece.
> 

I updated comment with CCI info, did pgindent run and renamed new 
function to SetRelationTableSpace(). New patch is attached.

> +INSERT INTO regress_tblspace_test_tbl (num1, num2, t)
> +  SELECT round(random()*100), random(), repeat('text', 1000000)
> +  FROM generate_series(1, 10) s(i);
> Repeating 1M times a text value is too costly for such a test.  And as
> even for empty tables there is one page created for toast indexes,
> there is no need for that?
> 

Yes, TOAST relation is created anyway. I just wanted to put some data 
into a TOAST index, so REINDEX did some meaningful work there, not only 
a new relfilenode creation. However you are right and this query 
increases tablespace tests execution for more for more than 2 times on 
my machine. I think that it is not really required.

> 
> This patch is introducing three new checks for system catalogs:
> - don't use tablespace for mapped relations.
> - don't use tablespace for system relations, except if
> allowSystemTableMods.
> - don't move non-shared relation to global tablespace.
> For the non-concurrent case, all three checks are in reindex_index().
> For the concurrent case, the two first checks are in
> ReindexMultipleTables() and the third one is in
> ReindexRelationConcurrently().  That's rather tricky to follow because
> CONCURRENTLY is not allowed on system relations.  I am wondering if it
> would be worth an extra comment effort, or if there is a way to
> consolidate that better.
> 

Yeah, all these checks we complicated from the beginning. I will try to 
find a better place tomorrow or put more info into the comments at 
least.

I am also going to check/fix the remaining points regarding 002 
tomorrow.


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company