|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
>> and assignment in the newly added SetRelTableSpace(). Result is
>> attached as
>> two separate patches for an ease of review, but no objections to merge
>> and apply at once if everything is fine.
> extern void SetRelationHasSubclass(Oid relationId, bool
> +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
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
> - 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
I am also going to check/fix the remaining points regarding 002
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
|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|