Date: Wed, 23 Dec 2020 16:47:49 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Michael Paquier <michael@paquier.xyz>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.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: <20201223224749.GT30237@telsasoft.com>
References: <X+Lz6l/mz3cxA/Cp@paquier.xyz>
 <20201223222205.GA13552@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20201223222205.GA13552@alvherre.pgsql>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Dec 23, 2020 at 07:22:05PM -0300, Alvaro Herrera wrote:
> Also: it seems a bit weird to me to put the flags inside the options
> struct.  I would keep them separate -- so initially the options struct
> would only have the tablespace OID, on API cleanliness grounds:

I don't see why they'd be separate or why it's cleaner ?

If the user says REINDEX (CONCURRENTLY, VERBOSE, TABLESPACE ts) , why would we
pass around the boolean flags separately from the other options ?

> struct ReindexOptions
> {
> 	tablepaceOid	oid;
> };
> extern bool
> reindex_relation(Oid relid, bits32 flags, ReindexOptions *options);


> But also, are we really envisioning that these routines would have all
> that many additional options?  Maybe it is sufficient to do just
> 
> extern bool
> reindex_relation(Oid relid, bits32 flags, tablespaceOid Oid);

That's what we did initially, and Michael suggested to put it into a struct.
Which makes the tablespace patches cleaner for each of REINDEX, CLUSTER,
VACUUM, since it doesn't require modifying the signature of 5-10 functions.
And future patches get to reap the benefit.

These are intended to be like VacuumParams.  Consider that ClusterOptions is
proposed to get not just a tablespaceOid but also an idxtablespaceOid.

This was getting ugly:

extern void reindex_index(Oid indexId, bool skip_constraint_checks,                                                                                                        
                          char relpersistence, int options, Oid tablespaceOid);                                                                     

-- 
Justin