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

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
Date: 2020-03-26 17:09:15
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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,
>>> 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.
> 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
> *relation*
> which is dropped/recreated multiple times in a transaction, but it's
> *not*
> prepared to deal with a given RelFileNode(Backend) being
> dropped/recreated,
> 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
> parameter
> to RelationSetNewRelfilenode().

Yes, it is not clear for me too.

>> 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).
> I'd suggest to keep the CLUSTER/VACUUM FULL separate from REINDEX, in
> case
> Michael or someone else wants to progress one but cannot commit to
> both.

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
TABLESPACE pg_default;
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
TABLESPACE pg_default;
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.

Alexey Kondratov

Postgres Professional
Russian Postgres Company

Attachment Content-Type Size
v12-0003-Allow-CLUSTER-and-VACUUM-FULL-to-change-tablespa.patch text/x-diff 24.8 KB
v12-0002-Specially-handle-toast-relations-during-REINDEX.patch text/x-diff 5.1 KB
v12-0001-Allow-REINDEX-to-change-tablespace.patch text/x-diff 34.1 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
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