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

On Wed, Jan 27, 2021 at 01:00:50AM +0300, Alexey Kondratov wrote:
> CheckRelationTableSpaceMove() does not feel like a right place for invoking
> post alter hooks. It is intended only to check for tablespace change
> possibility. Anyway, ATExecSetTableSpace() and
> ATExecSetTableSpaceNoStorage() already do that in the no-op case.
>
> I have removed this InvokeObjectPostAlterHook() from your 0001 and made 0002
> to work on top of it. I think that now it should look closer to what you
> described above.

Yeah, I was a bit hesitating about this part as those new routines
would not be used by ALTER-related commands in the next steps. Your
patch got that midway in-between though, by adding the hook to
SetRelationTableSpace but not to CheckRelationTableSpaceMove(). For
now, I have kept the hook out of those new routines because using an
ALTER hook for a utility command is inconsistent. Perhaps we'd want a
separate hook type dedicated to utility commands in objectaccess.c.

I have double-checked the code, and applied it after a few tweaks.

> In the new 0002 I moved ACL check to the upper level, i.e. ExecReindex(),
> and removed expensive text generation in test. Not touched yet some of your
> previously raised concerns. Also, you made SetRelationTableSpace() to accept
> Relation instead of Oid, so now we have to open/close indexes in the
> ReindexPartitions(), I am not sure that I use proper locking there, but it
> works.

Passing down Relation to the new routines makes the most sense to me
because we force the callers to think about the level of locking
that's required when doing any tablespace moves.

+ Relation iRel = index_open(partoid, ShareLock);
+
+ if (CheckRelationTableSpaceMove(iRel, params->tablespaceOid))
+ SetRelationTableSpace(iRel,
+ params->tablespaceOid,
+ InvalidOid);
Speaking of which, this breaks the locking assumptions of
SetRelationTableSpace(). I feel that we should think harder about
this part for partitioned indexes and tables because this looks rather
unsafe in terms of locking assumptions with partition trees. If we
cannot come up with a safe solution, I would be fine with disallowing
TABLESPACE in this case, as a first step. Not all problems have to be
solved at once, and even without this part the feature is still
useful.

+ /* It's not a shared catalog, so refuse to move it to shared tablespace */
+ if (params->tablespaceOid == GLOBALTABLESPACE_OID)
+ ereport(ERROR,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move non-shared relation to tablespace \"%s\"",
+ get_tablespace_name(params->tablespaceOid))));
Why is that needed if CheckRelationTableSpaceMove() is used?

bits32 options; /* bitmask of REINDEXOPT_* */
+ Oid tablespaceOid; /* tablespace to rebuild index */
} ReindexParams;
Mentioned upthread, but here I think that we should tell that
InvalidOid => keep the existing tablespace.
--
Michael