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

On 2020-02-11 19:48, Justin Pryzby wrote:
> For your v7 patch, which handles REINDEX to a new tablespace, I have a 
> few
> minor comments:
> 
> + * the relation will be rebuilt.  If InvalidOid is used, the default
> 
> => should say "currrent", not default ?
> 

Yes, it keeps current index tablespace in that case, thanks.

> 
> +++ b/doc/src/sgml/ref/reindex.sgml
> +    <term><literal>TABLESPACE</literal></term>
> ...
> +    <term><replaceable 
> class="parameter">new_tablespace</replaceable></term>
> 
> => I saw you split the description of TABLESPACE from new_tablespace 
> based on
> comment earlier in the thread, but I suggest that the descriptions for 
> these
> should be merged, like:
> 
> +   <varlistentry>
> +    <term><literal>TABLESPACE</literal><replaceable
> class="parameter">new_tablespace</replaceable></term>
> +    <listitem>
> +     <para>
> +      Allow specification of a tablespace where all rebuilt indexes
> will be created.
> +      Cannot be used with "mapped" relations. If 
> <literal>SCHEMA</literal>,
> +      <literal>DATABASE</literal> or <literal>SYSTEM</literal> are
> specified, then
> +      all unsuitable relations will be skipped and a single
> <literal>WARNING</literal>
> +      will be generated.
> +     </para>
> +    </listitem>
> +   </varlistentry>
> 

It sounds good to me, but here I just obey the structure, which is used 
all around. Documentation of ALTER TABLE/DATABASE, REINDEX and many 
others describes each literal/parameter in a separate entry, e.g. 
new_tablespace. So I would prefer to keep it as it is for now.

> 
> The existing patch is very natural, especially the parts in the 
> original patch
> handling vacuum full and cluster.  Those were removed to concentrate on
> REINDEX, and based on comments that it might be nice if ALTER handled 
> CLUSTER
> and VACUUM FULL.  On a separate thread, I brought up the idea of ALTER 
> using
> clustered order.  Tom pointed out some issues with my implementation, 
> but
> didn't like the idea, either.
> 
> So I suggest to re-include the CLUSTER/VAC FULL parts as a separate 
> 0002 patch,
> the same way they were originally implemented.
> 
> BTW, I think if "ALTER" were updated to support REINDEX (to allow 
> multiple
> operations at once), it might be either:
> |ALTER INDEX i SET TABLESPACE , REINDEX -- to reindex a single index
> on a given tlbspc
> or
> |ALTER TABLE tbl REINDEX USING INDEX TABLESPACE spc; -- to reindex all
> inds on table inds moved to a given tblspc
> "USING INDEX TABLESPACE" is already used for ALTER..ADD column/table 
> CONSTRAINT.
> 

Yes, I also think that allowing REINDEX/CLUSTER/VACUUM FULL to put 
resulting relation in a different tablespace is a very natural 
operation. However, I did a couple of attempts to integrate latter two 
with ALTER TABLE and failed with it, since it is already complex enough. 
I am still willing to proceed with it, but not sure how soon it will be.

Anyway, new version is attached. It is rebased in order to resolve 
conflicts with a recent fix of REINDEX CONCURRENTLY + temp relations, 
and includes this small comment fix.


Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
The Russian Postgres Company