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

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.

0001:
— It is mostly the same, but refactored
— I also included your most recent fix for REINDEX DATABASE with
allow_system_table_mods=1
— With this patch REINDEX + TABLESPACE simply errors out, when index on
TOAST table is met and allow_system_table_mods=0

0002:
— 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
performed.

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?

0003:
— 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 https://www.postgrespro.com
Russian Postgres Company