Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly - Mailing list pgsql-hackers
From | Alexey Kondratov |
---|---|
Subject | Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly |
Date | |
Msg-id | cb874a90-3946-91e4-4c31-5075f425b280@postgrespro.ru Whole thread Raw |
In response to | Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly (Justin Pryzby <pryzby@telsasoft.com>) |
Responses |
Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly
|
List | pgsql-hackers |
Hi Justin, 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; > REINDEX Yeah, we definitely should obey the same rules as ALTER TABLE / INDEX in my opinion. > 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. Regards -- Alexey Kondratov Postgres Professional https://www.postgrespro.com Russian Postgres Company
pgsql-hackers by date: