Date: Thu, 21 Jan 2021 15:26:51 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Steve Singer <steve@ssinger.info>,
	pgsql-hackers@lists.postgresql.org,
	Robert Haas <robertmhaas@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
Message-ID: <20210121212651.GY8560@telsasoft.com>
References: <03f88f70618ce73e75837b6125a143f7@postgrespro.ru>
 <20210120183439.GA21339@alvherre.pgsql>
 <YAjb2kYBKH07H5Xv@paquier.xyz>
 <bac6739d47f053fe04fbd8cf05283137@postgrespro.ru>
 <e8f209aebf33f02ea118e509a935647a@postgrespro.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <e8f209aebf33f02ea118e509a935647a@postgrespro.ru>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Thu, Jan 21, 2021 at 11:48:08PM +0300, Alexey Kondratov wrote:
> Attached is a new patch set of first two patches, that should resolve all
> the issues raised before (ACL, docs, tests) excepting TOAST. Double thanks
> for suggestion to add more tests with nested partitioning. I have found and
> squashed a huge bug related to the returning back to the default tablespace
> using newly added tests.
> 
> Regarding TOAST. Now we skip moving toast indexes or throw error if someone
> wants to move TOAST index directly. I had a look on ALTER TABLE SET
> TABLESPACE and it has a bit complicated logic:
> 
> 1) You cannot move TOAST table directly.
> 2) But if you move basic relation that TOAST table belongs to, then they are
> moved altogether.
> 3) Same logic as 2) happens if one does ALTER TABLE ALL IN TABLESPACE ...
> 
> That way, ALTER TABLE allows moving TOAST tables (with indexes) implicitly,
> but does not allow doing that explicitly. In the same time I found docs to
> be vague about such behavior it only says:
> 
>     All tables in the current database in a tablespace can be moved
>     by using the ALL IN TABLESPACE ... Note that system catalogs are
>     not moved by this command
> 
>     Changing any part of a system catalog table is not permitted.
> 
> So actually ALTER TABLE treats TOAST relations as system sometimes, but
> sometimes not.
> 
> From the end user perspective it makes sense to move TOAST with main table
> when doing ALTER TABLE SET TABLESPACE. But should we touch indexes on TOAST
> table with REINDEX? We cannot move TOAST relation itself, since we are doing
> only a reindex, so we end up in the state when TOAST table and its index are
> placed in the different tablespaces. This state is not reachable with ALTER
> TABLE/INDEX, so it seem we should not allow it with REINDEX as well, should
> we?

> +		 * Even if a table's indexes were moved to a new tablespace, the index
> +		 * on its toast table is not normally moved.
>  		 */
>  		ReindexParams newparams = *params;
>  
>  		newparams.options &= ~(REINDEXOPT_MISSING_OK);
> +		if (!allowSystemTableMods)
> +			newparams.tablespaceOid = InvalidOid;

I think you're right.  So actually TOAST should never move, even if
allowSystemTableMods, right ?

> @@ -292,7 +315,11 @@ REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { IN
>     with <command>REINDEX INDEX</command> or <command>REINDEX TABLE</command>,
>     respectively. Each partition of the specified partitioned relation is
>     reindexed in a separate transaction. Those commands cannot be used inside
> -   a transaction block when working on a partitioned table or index.
> +   a transaction block when working on a partitioned table or index. If
> +   <command>REINDEX</command> with <literal>TABLESPACE</literal> executed
> +   on partitioned relation fails it may have moved some partitions to the new
> +   tablespace. Repeated command will still reindex all partitions even if they
> +   are already in the new tablespace.

Minor corrections here:

If a <command>REINDEX</command> command fails when run on a partitioned
relation, and <literal>TABLESPACE</literal> was specified, then it may have
moved indexes on some partitions to the new tablespace.  Re-running the command
will reindex all partitions and move previously-unprocessed indexes to the new
tablespace.


-- 
Justin