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

On Wed, Dec 23, 2020 at 07:07:54PM -0600, Justin Pryzby wrote:
> On Wed, Dec 23, 2020 at 09:14:18PM -0300, Alvaro Herrera wrote:
>> On 2020-Dec-23, Justin Pryzby wrote: 
>>> This was getting ugly:
>>> 
>>> extern void reindex_index(Oid indexId, bool skip_constraint_checks,
>>>                           char relpersistence, int options, Oid tablespaceOid)
>> 
>> Is this what I suggested?

No idea if this is what you suggested, but it would be annoying to
have to change this routine's signature each time we need to pass down
a new option.

> No ; that was from an earlier revision of the patch adding the tablespace,
> before Michael suggested a ReindexOptions struct, which subsumes 'options' and
> 'tablespaceOid'.
> 
> I see now that 'skip_constraint_checks' is from REINDEX_REL_CHECK_CONSTRAINTS.
> It seems like that should be a REINDEXOPT_* flag, rather than REINDEX_REL_*,
> so doesn't need to be a separate boolean.  See also: 2d3320d3d.

FWIW, it still makes the most sense to me to keep the options that are
extracted from the grammar or things that apply to all the
sub-routines of REINDEX to be tracked in a single structure, so this
should include only the REINDEXOPT_* set for now, with the tablespace
OID as of this thread, and also the reindex filtering options.
REINDEX_REL_* is in my opinion of a different family because they only
apply to reindex_relation(), and partially to reindex_index(), so they
are very localized.  In short, anything in need of only
reindex_relation() has no need to know about the whole ReindexOption
business.
--
Michael