Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe fly - Mailing list 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.

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