Date: Tue, 11 Feb 2020 10:48:48 -0600
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>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
Message-ID: <20200211164848.GO1412@telsasoft.com>
References: <8a8f5f73-00d3-55f8-7583-1375ca8f6a91@postgrespro.ru>
 <fe8c0444-ebfc-ce5d-8843-7f93c0f53d7f@postgrespro.ru>
 <157395200750.29912.1178609357962324139.pgcf@coridan.postgresql.org>
 <827a9139-e02b-2dbf-5c6c-a6fbcaa1739a@postgrespro.ru>
 <CA+fd4k4uO5HS8Rb0PgH5oWQYT9xWYONZmLGUyAGvANFke-8c3Q@mail.gmail.com>
 <20191127035416.GG5435@paquier.xyz>
 <3ae48673-283c-3e99-3dd8-36ebb81614b5@postgrespro.ru>
 <20191202082134.GI1696@paquier.xyz>
 <c8ed43871e171ca2077aa9e1d0ed970a@postgrespro.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c8ed43871e171ca2077aa9e1d0ed970a@postgrespro.ru>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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