|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 2020-03-26 02:40, Justin Pryzby wrote:
> On Thu, Mar 12, 2020 at 08:08:46PM +0300, Alexey Kondratov wrote:
>> 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:
>>>> 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,
>>> 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
>>> tablespace param to RelationSetNewRelfilenode.
>> Do you have any understanding of what exactly causes this error? I
>> 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.
> The PANIC is from smgr hashtable, which couldn't find an entry it
> expected. My
> very tentative understanding is that smgr is prepared to handle a
> which is dropped/recreated multiple times in a transaction, but it's
> prepared to deal with a given RelFileNode(Backend) being
> since that's used as a hash key.
> I revisited it and solved it in a somewhat nicer way.
I included your new solution regarding this part from 0004 into 0001. It
seems that at least a tip of the problem was in that we tried to change
tablespace to pg_default being already there.
> It's still not clear to
> me if there's an issue with your original way of adding a tablespace
> to RelationSetNewRelfilenode().
Yes, it is not clear for me too.
>> Many thanks for you review and fixups! There are some inconsistencies
>> 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
>> separated for now, since this part requires more understanding IMO
>> comparison with v4 implementation).
> I'd suggest to keep the CLUSTER/VACUUM FULL separate from REINDEX, in
> Michael or someone else wants to progress one but cannot commit to
Yes, sure, I did not have plans to melt everything into a single patch.
So, it has taken much longer to understand and rework all these fixes
and permission validations. Attached is the updated patch set.
— It is mostly the same, but refactored
— I also included your most recent fix for REINDEX DATABASE with
— With this patch REINDEX + TABLESPACE simply errors out, when index on
TOAST table is met and allow_system_table_mods=0
— I reworked it a bit, since REINDEX CONCURRENTLY is not allowed on
system catalog anyway, that is checked at the hegher levels of statement
processing. So we have to care about TOAST relations
— Also added the same check into the plain REINDEX
— It works fine, but I am not entirely happy that with this patch
errors/warnings are a bit inconsistent:
template1=# REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_12773_index
WARNING: skipping tablespace change of "pg_toast_12773_index"
DETAIL: Cannot move system relation, only REINDEX CONCURRENTLY is
template1=# REINDEX TABLE CONCURRENTLY pg_toast.pg_toast_12773
ERROR: permission denied: "pg_toast_12773" is a system catalog
And REINDEX DATABASE CONCURRENTLY will generate a warning again.
Maybe we should always throw a warning and do only reindex if it is not
possible to change tablespace?
— I have get rid of some of previous refactoring pieces like
check_relation_is_movable for now. Let all these validations to settle
and then think whether we could do it better
— Added CLUSTER to copy/equalfuncs
— Cleaned up messages and comments
I hope that I did not forget anything from your proposals.
Postgres Professional https://www.postgrespro.com
Russian Postgres Company
|Next Message||Ashutosh Bapat||2020-03-26 17:10:57||Re: A bug when use get_bit() function for a long bytea string|
|Previous Message||Vik Fearing||2020-03-26 16:58:50||Tab completion for \gx|