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

Justin Pryzby <pryzby@telsasoft.com>

From: Justin Pryzby <pryzby@telsasoft.com>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Michael Paquier <michael@paquier.xyz>, Masahiko Sawada <masahiko.sawada@2ndquadrant.com>, Steve Singer <steve@ssinger.info>, pgsql-hackers@lists.postgresql.org, Alvaro Herrera <alvherre@2ndquadrant.com>, Robert Haas <robertmhaas@gmail.com>, Alexander Korotkov <a.korotkov@postgrespro.ru>, Masahiko Sawada <sawada.mshk@gmail.com>, Jose Luis Tallon <jltallon@adv-solutions.net>
Date: 2020-02-11T16:48:48Z
Lists: pgsql-hackers
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 ?

+++ 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>

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.

-- 
Justin



Commits

  1. Add TABLESPACE option to REINDEX

  2. Refactor code in tablecmds.c to check and process tablespace moves

  3. Refactor option handling of CLUSTER, REINDEX and VACUUM

  4. pg_dump: Don't use enums for defining bit mask values

  5. Refactor CLUSTER and REINDEX grammar to use DefElem for option lists

  6. Refactor parsing rules for option lists of EXPLAIN, VACUUM and ANALYZE

  7. Improve tab completion of REINDEX in psql

  8. Fix possible crash during FATAL exit from reindexing.