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

On Fri, Dec 04, 2020 at 09:40:31PM +0300, Alexey Kondratov wrote:
> > I liked the bools, but dropped them so the patch is smaller.
>
> I had a look on 0001 and it looks mostly fine to me except some strange
> mixture of tabs/spaces in the ExecReindex(). There is also a couple of
> meaningful comments:
>
> - options =
> - (verbose ? REINDEXOPT_VERBOSE : 0) |
> - (concurrently ? REINDEXOPT_CONCURRENTLY : 0);
> + if (verbose)
> + params.options |= REINDEXOPT_VERBOSE;
>
> Why do we need this intermediate 'verbose' variable here? We only use it
> once to set a bitmask. Maybe we can do it like this:
>
> params.options |= defGetBoolean(opt) ?
> REINDEXOPT_VERBOSE : 0;

That allows *setting* REINDEXOPT_VERBOSE, but doesn't *unset* it if someone
runs (VERBOSE OFF). So I kept the bools like Michael originally had rather
than writing "else: params.options &= ~REINDEXOPT_VERBOSE"

> See also attached txt file with diff (I wonder can I trick cfbot this way,
> so it does not apply the diff).

Yes, I think that works :)
I believe it looks for *.diff and *.patch.

> + int options; /* bitmask of lowlevel REINDEXOPT_* */
>
> I would prefer if the comment says '/* bitmask of ReindexOption */' as in
> the VacuumOptions, since citing the exact enum type make it easier to
> navigate source code.

Yes, thanks.

This also fixes some minor formatting and rebase issues, including broken doc/.

--
Justin