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

On 2019-12-02 11:21, Michael Paquier wrote:
> On Wed, Nov 27, 2019 at 08:47:06PM +0300, Alexey Kondratov wrote:
> 
>> Thus, I cannot get your point correctly here. Can you, please, 
>> elaborate a
>> little bit more your concerns?
> 
> The case of REINDEX CONCURRENTLY is pretty simple, because a new
> relation which is a copy of the old relation is created before doing
> the reindex, so you simply need to set the tablespace OID correctly
> in index_concurrently_create_copy().  And actually, I think that the
> computation is incorrect because we need to check after
> MyDatabaseTableSpace as well, no?
> 
> The case of REINDEX is more tricky, because you are working on a
> relation that already exists, hence I think that what you need to do a
> different thing before the actual REINDEX:
> 1) Update the existing relation's pg_class tuple to point to the new
> tablespace.
> 2) Do a CommandCounterIncrement.
> So I think that the order of the operations you are doing is incorrect,
> and that you have a risk of breaking the existing tablespace assignment
> logic done when first flushing a new relfilenode.
> 
> This actually brings an extra thing: when doing a plain REINDEX you
> need to make sure that the past relfilenode of the relation gets away
> properly.  The attached POC patch does that before doing the CCI which
> is a bit ugly, but that's enough to show my point, and there is no
> need to touch RelationSetNewRelfilenode() this way.
> 

OK, I hope that now I understand your concerns better. Another thing I 
just realised is that RelationSetNewRelfilenode is also used for mapped 
relations, which are not movable at all, so adding a tablespace options 
there seems to be not semantically correct as well. However, I still 
have not find a way to reproduce how to actually brake anything with my 
previous version of the patch.

As for doing RelationDropStorage before CCI, I do not think that there 
is something wrong with it, this is exactly what 
RelationSetNewRelfilenode does. I have only moved RelationDropStorage 
before CatalogTupleUpdate compared to your proposal to match order 
inside RelationSetNewRelfilenode.

> 
> Your patch has forgotten to update copyfuncs.c and equalfuncs.c with
> the new tablespace string field.
> 
> It would be nice to add tab completion for this new clause in psql.
> This is not ready for committer yet in my opinion, and more work is
> done, so I am marking it as returned with feedback for now.
> 

Finally, I have also merged and unified all your and Masahiko's 
proposals with my recent changes: ereport corrections, tab-completion, 
docs update, copy/equalfuncs update, etc. New version is attached. Have 
it come any closer to a committable state now?


Regards
-- 
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company