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

On 2021-01-26 09:58, Michael Paquier wrote:
> On Mon, Jan 25, 2021 at 11:11:38PM +0300, Alexey Kondratov wrote:
>> I updated comment with CCI info, did pgindent run and renamed new
>> function
>> to SetRelationTableSpace(). New patch is attached.
>>
>> [...]
>>
>> Yeah, all these checks we complicated from the beginning. I will try
>> to find
>> a better place tomorrow or put more info into the comments at least.
>
> I was reviewing that, and I think that we can do a better
> consolidation on several points that will also help the features
> discussed on this thread for VACUUM, CLUSTER and REINDEX.
>
> If you look closely, ATExecSetTableSpace() uses the same logic as the
> code modified here to check if a relation can be moved to a new
> tablespace, with extra checks for mapped relations,
> GLOBALTABLESPACE_OID or if attempting to manipulate a temp relation
> from another session. There are two differences though:
> - Custom actions are taken between the phase where we check if a
> relation can be moved to a new tablespace, and the update of
> pg_class.
> - ATExecSetTableSpace() needs to be able to set a given relation
> relfilenode on top of reltablespace, the newly-created one.
>
> So I think that the heart of the problem is made of two things here:
> - We should have one common routine for the existing code paths and
> the new code paths able to check if a tablespace move can be done or
> not. The case of a cluster, reindex or vacuum on a list of relations
> extracted from pg_class would still require a different handling
> as incorrect relations have to be skipped, but the case of individual
> relations can reuse the refactoring pieces done here
> (see CheckRelationTableSpaceMove() in the attached).
> - We need to have a second routine able to update reltablespace and
> optionally relfilenode for a given relation's pg_class entry, once the
> caller has made sure that CheckRelationTableSpaceMove() validates a
> tablespace move.
>

I think that I got your idea. One comment:

+bool
+CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId)
+{
+ Oid oldTableSpaceId;
+ Oid reloid = RelationGetRelid(rel);
+
+ /*
+ * No work if no change in tablespace. Note that MyDatabaseTableSpace
+ * is stored as 0.
+ */
+ oldTableSpaceId = rel->rd_rel->reltablespace;
+ if (newTableSpaceId == oldTableSpaceId ||
+ (newTableSpaceId == MyDatabaseTableSpace && oldTableSpaceId == 0))
+ {
+ InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+ return false;
+ }

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.

> Please note that was a bug in your previous patch 0002: shared
> dependencies need to be registered if reltablespace is updated of
> course, but also iff the relation has no physical storage. So
> changeDependencyOnTablespace() requires a check based on
> RELKIND_HAS_STORAGE(), or REINDEX would have registered shared
> dependencies even for relations with storage, something we don't
> want per the recent work done by Alvaro in ebfe2db.
>

Yes, thanks.

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.

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.

Regards
--
Alexey Kondratov

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