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

On 2020-12-04 04:25, Justin Pryzby wrote:
> On Thu, Dec 03, 2020 at 04:12:53PM +0900, Michael Paquier wrote:
>> > +typedef struct ReindexParams {
>> > + bool concurrently;
>> > + bool verbose;
>> > + bool missingok;
>> > +
>> > + int options; /* bitmask of lowlevel REINDEXOPT_* */
>> > +} ReindexParams;
>> > +
>>
>> By moving everything into indexcmds.c, keeping ReindexParams within it
>> makes sense to me. Now, there is no need for the three booleans
>> because options stores the same information, no?
>
> 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;

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

+ 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.

>
> Regarding the REINDEX patch, I think this comment is misleading:
>
> |+ * Even if table was moved to new tablespace,
> normally toast cannot move.
> | */
> |+ Oid toasttablespaceOid = allowSystemTableMods ?
> tablespaceOid : InvalidOid;
> | result |= reindex_relation(toast_relid, flags,
>
> I think it ought to say "Even if a table's indexes were moved to a new
> tablespace, its toast table's index is not normally moved"
> Right ?
>

Yes, I think so, we are dealing only with index tablespace changing
here. Thanks for noticing.

>
> Also, I don't know whether we should check for GLOBALTABLESPACE_OID
> after
> calling get_tablespace_oid(), or in the lowlevel routines. Note that
> reindex_relation is called during cluster/vacuum, and in the later
> patches, I
> moved the test from from cluster() and ExecVacuum() to
> rebuild_relation().
>

IIRC, I wanted to do GLOBALTABLESPACE_OID check as early as possible
(just after getting Oid), since it does not make sense to proceed
further if tablespace is set to that value. So initially there were a
lot of duplicative GLOBALTABLESPACE_OID checks, since there were a lot
of reindex entry-points (index, relation, concurrently, etc.). Now we
are going to have ExecReindex(), so there are much less entry-points and
in my opinion it is fine to keep this validation just after
get_tablespace_oid().

However, this is mostly a sanity check. I can hardly imagine a lot of
users trying to constantly move indexes to the global tablespace, so it
is also OK to put this check deeper into guts.

Regards
--
Alexey Kondratov

Postgres Professional https://www.postgrespro.com
Russian Postgres Company