|From:||Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>|
|To:||Justin Pryzby <pryzby(at)telsasoft(dot)com>|
|Cc:||Michael Paquier <michael(at)paquier(dot)xyz>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, 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 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;
Yeah, we definitely should obey the same rules as ALTER TABLE / INDEX in
> 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.
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
|Next Message||Tom Lane||2020-03-12 17:19:56||Re: ALTER tbl rewrite loses CLUSTER ON index|
|Previous Message||Tom Lane||2020-03-12 16:39:41||Re: [PATCH] Use PKG_CHECK_MODULES to detect the libxml2 library|