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

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. It's still not clear to
me if there's an issue with your original way of adding a tablespace parameter
to RelationSetNewRelfilenode().

> Probably you mean v4 patch. Yes, interestingly, if we do everything at once
> inside RelationSetNewRelfilenode(), then there is no issue at all with:

Yes, I meant to say "worth revisiting the v4 patch".

> 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. But
probably we should plan to finish this in July.

--
Justin