|From:||Alexey Kondratov <a(dot)kondratov(at)postgrespro(dot)ru>|
|To:||Justin Pryzby <pryzby(at)telsasoft(dot)com>|
|Cc:||Michael Paquier <michael(at)paquier(dot)xyz>, Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com>, Steve Singer <steve(at)ssinger(dot)info>, pgsql-hackers(at)lists(dot)postgresql(dot)org, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Jose Luis Tallon <jltallon(at)adv-solutions(dot)net>|
|Subject:||Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on the fly|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
On 2020-02-11 19:48, Justin Pryzby wrote:
> For your v7 patch, which handles REINDEX to a new tablespace, I have a
> 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
> => 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
> should be merged, like:
> + <varlistentry>
> + <term><literal>TABLESPACE</literal><replaceable
> + <listitem>
> + <para>
> + Allow specification of a tablespace where all rebuilt indexes
> will be created.
> + Cannot be used with "mapped" relations. If
> + <literal>DATABASE</literal> or <literal>SYSTEM</literal> are
> specified, then
> + all unsuitable relations will be skipped and a single
> + 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
> and VACUUM FULL. On a separate thread, I brought up the idea of ALTER
> clustered order. Tom pointed out some issues with my implementation,
> 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
> operations at once), it might be either:
> |ALTER INDEX i SET TABLESPACE , REINDEX -- to reindex a single index
> on a given tlbspc
> |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
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.
Postgres Professional https://www.postgrespro.com
The Russian Postgres Company
|Next Message||Fabien COELHO||2020-02-29 14:29:19||Re: pgbench: option delaying queries till connections establishment?|
|Previous Message||Tomas Vondra||2020-02-29 12:24:41||Re: SLRU statistics|