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

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.

> > struct ReindexIndexCallbackState
> > {
> > - int options; /* options from statement */
> > + bool concurrently;
> > Oid locked_table_oid; /* tracks previously locked table */
> > };
>
> Here also, I think that we should just pass down the full
> ReindexParams set.

Ok.

Regarding the REINDEX patch, I think this comment is misleading:

| /*
| * If the relation has a secondary toast rel, reindex that too while we
| * still hold the lock on the main table.
| */
| if ((flags & REINDEX_REL_PROCESS_TOAST) && OidIsValid(toast_relid))
| {
| /*
| * Note that this should fail if the toast relation is missing, so
| * reset REINDEXOPT_MISSING_OK.
|+ *
|+ * Even if table was moved to new tablespace, normally toast cannot move.
| */
|+ Oid toasttablespaceOid = allowSystemTableMods ? tablespaceOid : InvalidOid;
| result |= reindex_relation(toast_relid, flags,
|- options & ~(REINDEXOPT_MISSING_OK));
|+ options & ~(REINDEXOPT_MISSING_OK),
|+ toasttablespaceOid);
| }

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 ?

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().

--
Justin