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

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 also changed the errcode and detail for this one.
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("incompatible TABLESPACE option"),
errdetail("TABLESPACE can only be used with VACUUM FULL.")));

--
Justin