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