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