Date: Wed, 23 Dec 2020 19:22:05 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Michael Paquier <michael@paquier.xyz>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	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: <20201223222205.GA13552@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <X+Lz6l/mz3cxA/Cp@paquier.xyz>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Dec-23, Michael Paquier wrote:

>  bool
> -reindex_relation(Oid relid, int flags, int options)
> +reindex_relation(Oid relid, int flags, ReindexOptions *options)
>  {
>  	Relation	rel;
>  	Oid			toast_relid;

Wait a minute.  reindex_relation has 'flags' and *also* 'options' with
an embedded 'flags' member?  Surely that's not right.  I see that they
carry orthogonal sets of options ... but why aren't they a single
bitmask instead of two separate ones?  This looks weird and confusing.


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:

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

I guess you could argue that it's more performance to set up only two
arguments to the function call instead of three .. but I doubt that's
measurable for anything in DDL-land.

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);