Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the 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