Date: Fri, 4 Dec 2020 16:28:26 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Justin Pryzby <pryzby@telsasoft.com>,
	Alexey Kondratov <a.kondratov@postgrespro.ru>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Steve Singer <steve@ssinger.info>,
	pgsql-hackers@lists.postgresql.org,
	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: <20201204192826.GA15417@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <X8nLCFK4Y60UGOJb@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Dec-04, Michael Paquier wrote:

> VacuumOption does that since 6776142, and ClusterOption since 9ebe057,
> so switching ReindexOption to just match the two others still looks
> like the most consistent move.

9ebe057 goes to show why this is a bad idea, since it has this:

+typedef enum ClusterOption
+{
+   CLUOPT_RECHECK,             /* recheck relation state */
+   CLUOPT_VERBOSE              /* print progress info */
+} ClusterOption;

and then you do things like

+                   if ($2)
+                       n->options |= CLUOPT_VERBOSE;

and then tests like

+       if ((options & VACOPT_VERBOSE) != 0)

Now if you were to ever define third and fourth values in that enum,
this would immediately start malfunctioning.

FWIW I'm with Peter on this.