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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: 2021-01-25 20:11:38
Message-ID: 690fa051803c071d213b0d07b5aa9f55@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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

Attachment Content-Type Size
v5-0001-Extract-common-part-from-ATExecSetTableSpaceNoSto.patch text/x-diff 4.9 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andres Freund 2021-01-25 20:48:13 Re: Snapbuild woes followup
Previous Message Andres Freund 2021-01-25 20:00:08 Re: Snapbuild woes followup