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
