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

From: Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>
To: Michael Paquier <michael(at)paquier(dot)xyz>
Cc: Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly
Date: 2020-01-04 18:38:24
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers

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?

Alexey Kondratov

Postgres Professional
Russian Postgres Company

Attachment Content-Type Size
v7-0001-Allow-REINDEX-to-change-tablespace.patch text/x-diff 32.9 KB

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Peter Geoghegan 2020-01-04 18:38:49 Re: pgsql: Add basic TAP tests for psql's tab-completion logic.
Previous Message Tom Lane 2020-01-04 18:19:28 Re: pgsql: Add basic TAP tests for psql's tab-completion logic.