Date: Wed, 27 Jan 2021 15:35:03 -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: <20210127213503.GX30745@telsasoft.com>
References: <bac6739d47f053fe04fbd8cf05283137@postgrespro.ru>
 <e8f209aebf33f02ea118e509a935647a@postgrespro.ru>
 <20210121212651.GY8560@telsasoft.com>
 <944df7cc452fe0c34cf7820da4588d05@postgrespro.ru>
 <YA58Qdgb3a7/yrOH@paquier.xyz>
 <690fa051803c071d213b0d07b5aa9f55@postgrespro.ru>
 <YA+9mAMWYLXJMVPL@paquier.xyz>
 <c36e51a07183b95ddd287be4ee4cfbbd@postgrespro.ru>
 <YBDamQI7NCr7JBJA@paquier.xyz>
 <ef752a011a4431a9a6ae6f8c1e2eb001@postgrespro.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ef752a011a4431a9a6ae6f8c1e2eb001@postgrespro.ru>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

Thanks for updating the patch.  I have just a couple comments on the new (and
old) language.

On Thu, Jan 28, 2021 at 12:19:06AM +0300, Alexey Kondratov wrote:
> Also added tests for ACL checks, relfilenode changes. Added ACL recheck for
> multi-transactional case. Added info about TOAST index reindexing. Changed
> some comments.

> +      Specifies that indexes will be rebuilt on a new tablespace.
> +      Cannot be used with "mapped" and system (unless <varname>allow_system_table_mods</varname>

say mapped *or* system relations
Or maybe:
mapped or (unless >allow_system_table_mods<) system relations.

> +      is set to <literal>TRUE</literal>) relations. If <literal>SCHEMA</literal>,
> +      <literal>DATABASE</literal> or <literal>SYSTEM</literal> are specified,
> +      then all "mapped" and system relations will be skipped and a single
> +      <literal>WARNING</literal> will be generated. Indexes on TOAST tables
> +      are reindexed, but not moved the new tablespace.

moved *to* the new tablespace.
I don't know if that needs to be said at all.  We talked about it a lot to
arrive at the current behavior, but I think that's only due to the difficulty
of correcting the initial mistake.

> +	/*
> +	 * Set the new tablespace for the relation.  Do that only in the
> +	 * case where the reindex caller wishes to enforce a new tablespace.

I'd say just "/* Set new tablespace, if requested */
You wrote something similar in an earlier revision of your refactoring patch.

> +		 * Mark the relation as ready to be dropped at transaction commit,
> +		 * before making visible the new tablespace change so as this won't
> +		 * miss things.

This comment is vague.  I think Michael first wrote this comment about a year
ago.  Does it mean "so the new tablespace won't be missed" ?  Missed by what ?

-- 
Justin