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

On Sat, Dec 12, 2020 at 01:45:26PM -0600, Justin Pryzby wrote:
> On Sat, Dec 12, 2020 at 09:20:35AM +0100, Peter Eisentraut wrote:
> > On 2020-12-11 21:27, Alvaro Herrera wrote:
> > > By the way--  What did you think of the idea of explictly marking the
> > > types used for bitmasks using types bits32 and friends, instead of plain
> > > int, which is harder to spot?
> > 
> > If we want to make it clearer, why not turn the thing into a struct, as in
> > the attached patch, and avoid the bit fiddling altogether.
> 
> I like this.
> It's a lot like what I wrote as [PATCH v31 1/5] ExecReindex and ReindexParams
> In my v31 patch, I moved ReindexOptions to a private structure in indexcmds.c,
> with an "int options" bitmask which is passed to reindex_index() et al.  Your
> patch keeps/puts ReindexOptions index.h, so it also applies to reindex_index,
> which I think is good.
> 
> So I've rebased this branch on your patch.
> 
> Some thoughts:
> 
>  - what about removing the REINDEXOPT_* prefix ?
>  - You created local vars with initialization like "={}". But I thought it's
>    needed to include at least one struct member like "={false}", or else
>    they're not guaranteed to be zerod ?
>  - You passed the structure across function calls.  The usual convention is to
>    pass a pointer.

I think maybe Michael missed this message (?)
I had applied some changes on top of Peter's patch.

I squished those commits now, and also handled ClusterOption and VacuumOption
in the same style.

Some more thoughts:
 - should the structures be named in plural ? "ReindexOptions" etc.  Since they
   define *all* the options, not just a single bit.
 - For vacuum, do we even need a separate structure, or should the members be
   directly within VacuumParams ?  It's a bit odd to write
   params.options.verbose.  Especially since there's also ternary options which
   are directly within params.
 - Then, for cluster, I think it should be called ClusterParams, and eventually
   include the tablespaceOid, like what we're doing for Reindex.

I am awaiting feedback on these before going further since I've done too much
rebasing with these ideas going back and forth and back.

-- 
Justin