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

On Tue, Nov 26, 2019 at 11:09:55PM +0100, Masahiko Sawada wrote:
> Thank you for working on this.

I have been looking at the latest patch as well.

> I looked at v4 patch. Here are some comments:
>
> + /* Skip all mapped relations if TABLESPACE is specified */
> + if (OidIsValid(tableSpaceOid) &&
> + classtuple->relfilenode == 0)
>
> I think we can use OidIsValid(classtuple->relfilenode) instead.

Yes, definitely.

> This change says that temporary relation is not supported but it
> actually seems to work. Which is correct?

Yeah, I don't really see a reason why it would not work.

> ISTM the kind of above errors are the same: the given tablespace
> exists but moving tablespace to it is not allowed since it's not
> supported in PostgreSQL. So I think we can use
> ERRCODE_FEATURE_NOT_SUPPORTED instead of
> ERRCODE_INVALID_PARAMETER_VALUE (which is used at 3 places) .

Yes, it is also not project style to use full sentences in error
messages, so I would suggest instead (note the missing quotes in the
original patch):
cannot move non-shared relation to tablespace \"%s\"

@@ -3455,6 +3461,8 @@ RelationSetNewRelfilenode(Relation relation,
char persistence)
*/
newrnode = relation->rd_node;
newrnode.relNode = newrelfilenode;
+ if (OidIsValid(tablespaceOid))
+ newrnode.spcNode = newTablespaceOid;
The core of the patch is actually here. It seems to me that this is a
very bad idea because you actually hijack a logic which happens at a
much lower level which is based on the state of the tablespace stored
in the relation cache entry of the relation being reindexed, then the
tablespace choice actually happens in RelationInitPhysicalAddr() which
for the new relfilenode once the follow-up CCI is done. So this very
likely needs more thoughts, and bringing to the point: shouldn't you
actually be careful that the relation tablespace is correctly updated
before reindexing it and before creating its new relfilenode? This
way, RelationSetNewRelfilenode() does not need any additional work,
and I think that this saves from potential bugs in the choice of the
tablespace used with the new relfilenode.

There is no need for opt_tablespace_name as new node for the parsing
grammar of gram.y as OptTableSpace is able to do the exact same job.

+ /* Skip all mapped relations if TABLESPACE is specified */
+ if (OidIsValid(tableSpaceOid) &&
+ classtuple->relfilenode == 0)
+ {
+ if (!system_warning)
+ ereport(WARNING,
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("cannot move indexes of system relations, skipping all")));
+ system_warning = true;
continue;
It seems to me that you need to use RelationIsMapped() here, and we
have no tests for it. On top of that, we should warn about *both*
for catalogs reindexes and mapped relation whose tablespaces are being
changed once each.

Your patch has forgotten to update copyfuncs.c and equalfuncs.c with
the new tablespace string field.

It would be nice to add tab completion for this new clause in psql.
This is not ready for committer yet in my opinion, and more work is
done, so I am marking it as returned with feedback for now.
--
Michael