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