Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace onthe 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
> +    TABLESPACE
> ...
> +     class="parameter">new_tablespace
> 
> => 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:
> 
> +   
> +    TABLESPACE class="parameter">new_tablespace
> +    
> +     
> +      Allow specification of a tablespace where all rebuilt indexes
> will be created.
> +      Cannot be used with "mapped" relations. If 
> SCHEMA,
> +      DATABASE or SYSTEM are
> specified, then
> +      all unsuitable relations will be skipped and a single
> WARNING
> +      will be generated.
> +     
> +    
> +   
> 

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