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

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