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