Back-patch "Refactor code in tablecmds.c to check and process tablespace moves"

Back-patch commits 4c9c359d38ff1e2de388eedd860785be6a49201c and
24843297a96d7be16cc3f4b090aacfc6e5e6839e to v13 and v12.  Before those
commits, we held the modifiable copy of the relation's pg_class row
throughout a table_relation_copy_data().  That can last long enough to
copy MaxBlockNumber of data.  A subsequent fix will hold LockTuple() for
the lifespan of that modifiable copy.  By back-patching this first, we
avoid a needless long-duration LOCKTAG_TUPLE.

Discussion: https://postgr.es/m/20231027214946.79.nmisch@google.com

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 78d529c22c022370379fd1cc628edce91b9f3937..ae21e9951826ae39745267d7bc0aa3fba6961aac 100644 (file)
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3027,6 +3027,112 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
        table_close(relationRelation, RowExclusiveLock);
 }
 
+/*
+ * CheckRelationTableSpaceMove
+ *             Check if relation can be moved to new tablespace.
+ *
+ * NOTE: The caller must hold AccessExclusiveLock on the relation.
+ *
+ * Returns true if the relation can be moved to the new tablespace; raises
+ * an error if it is not possible to do the move; returns false if the move
+ * would have no effect.
+ */
+bool
+CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId)
+{
+       Oid                     oldTableSpaceId;
+
+       /*
+        * 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))
+               return false;
+
+       /*
+        * We cannot support moving mapped relations into different tablespaces.
+        * (In particular this eliminates all shared catalogs.)
+        */
+       if (RelationIsMapped(rel))
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot move system relation \"%s\"",
+                                               RelationGetRelationName(rel))));
+
+       /* Cannot move a non-shared relation into pg_global */
+       if (newTableSpaceId == GLOBALTABLESPACE_OID)
+               ereport(ERROR,
+                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                                errmsg("only shared relations can be placed in pg_global tablespace")));
+
+       /*
+        * Do not allow moving temp tables of other backends ... their local
+        * buffer manager is not going to cope.
+        */
+       if (RELATION_IS_OTHER_TEMP(rel))
+               ereport(ERROR,
+                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+                                errmsg("cannot move temporary tables of other sessions")));
+
+       return true;
+}
+
+/*
+ * SetRelationTableSpace
+ *             Set new reltablespace and relfilenode in pg_class entry.
+ *
+ * newTableSpaceId is the new tablespace for the relation, and
+ * newRelFileNode its new filenode.  If newRelFileNode is InvalidOid,
+ * this field is not updated.
+ *
+ * NOTE: The caller must hold AccessExclusiveLock on the relation.
+ *
+ * The caller of this routine had better check if a relation can be
+ * moved to this new tablespace by calling CheckRelationTableSpaceMove()
+ * first, and is responsible for making the change visible with
+ * CommandCounterIncrement().
+ */
+void
+SetRelationTableSpace(Relation rel,
+                                         Oid newTableSpaceId,
+                                         Oid newRelFileNode)
+{
+       Relation        pg_class;
+       HeapTuple       tuple;
+       Form_pg_class rd_rel;
+       Oid                     reloid = RelationGetRelid(rel);
+
+       Assert(CheckRelationTableSpaceMove(rel, newTableSpaceId));
+
+       /* Get a modifiable copy of the relation's pg_class row. */
+       pg_class = table_open(RelationRelationId, RowExclusiveLock);
+
+       tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+       if (!HeapTupleIsValid(tuple))
+               elog(ERROR, "cache lookup failed for relation %u", reloid);
+       rd_rel = (Form_pg_class) GETSTRUCT(tuple);
+
+       /* Update the pg_class row. */
+       rd_rel->reltablespace = (newTableSpaceId == MyDatabaseTableSpace) ?
+               InvalidOid : newTableSpaceId;
+       if (OidIsValid(newRelFileNode))
+               rd_rel->relfilenode = newRelFileNode;
+       CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+
+       /*
+        * Record dependency on tablespace.  This is only required for relations
+        * that have no physical storage.
+        */
+       if (!RELKIND_HAS_STORAGE(rel->rd_rel->relkind))
+               changeDependencyOnTablespace(RelationRelationId, reloid,
+                                                                        rd_rel->reltablespace);
+
+       heap_freetuple(tuple);
+       table_close(pg_class, RowExclusiveLock);
+}
+
 /*
  *             renameatt_check                 - basic sanity checks before attribute rename
  */
@@ -13086,13 +13192,9 @@ static void
 ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
 {
        Relation        rel;
-       Oid                     oldTableSpace;
        Oid                     reltoastrelid;
        Oid                     newrelfilenode;
        RelFileNode newrnode;
-       Relation        pg_class;
-       HeapTuple       tuple;
-       Form_pg_class rd_rel;
        List       *reltoastidxids = NIL;
        ListCell   *lc;
 
@@ -13101,45 +13203,15 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
         */
        rel = relation_open(tableOid, lockmode);
 
-       /*
-        * No work if no change in tablespace.
-        */
-       oldTableSpace = rel->rd_rel->reltablespace;
-       if (newTableSpace == oldTableSpace ||
-               (newTableSpace == MyDatabaseTableSpace && oldTableSpace == 0))
+       /* Check first if relation can be moved to new tablespace */
+       if (!CheckRelationTableSpaceMove(rel, newTableSpace))
        {
                InvokeObjectPostAlterHook(RelationRelationId,
                                                                  RelationGetRelid(rel), 0);
-
                relation_close(rel, NoLock);
                return;
        }
 
-       /*
-        * We cannot support moving mapped relations into different tablespaces.
-        * (In particular this eliminates all shared catalogs.)
-        */
-       if (RelationIsMapped(rel))
-               ereport(ERROR,
-                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                errmsg("cannot move system relation \"%s\"",
-                                               RelationGetRelationName(rel))));
-
-       /* Can't move a non-shared relation into pg_global */
-       if (newTableSpace == GLOBALTABLESPACE_OID)
-               ereport(ERROR,
-                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                errmsg("only shared relations can be placed in pg_global tablespace")));
-
-       /*
-        * Don't allow moving temp tables of other backends ... their local buffer
-        * manager is not going to cope.
-        */
-       if (RELATION_IS_OTHER_TEMP(rel))
-               ereport(ERROR,
-                               (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
-                                errmsg("cannot move temporary tables of other sessions")));
-
        reltoastrelid = rel->rd_rel->reltoastrelid;
        /* Fetch the list of indexes on toast relation if necessary */
        if (OidIsValid(reltoastrelid))
@@ -13150,14 +13222,6 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
                relation_close(toastRel, lockmode);
        }
 
-       /* Get a modifiable copy of the relation's pg_class row */
-       pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
-       tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(tableOid));
-       if (!HeapTupleIsValid(tuple))
-               elog(ERROR, "cache lookup failed for relation %u", tableOid);
-       rd_rel = (Form_pg_class) GETSTRUCT(tuple);
-
        /*
         * Relfilenodes are not unique in databases across tablespaces, so we need
         * to allocate a new one in the new tablespace.
@@ -13188,18 +13252,13 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
         *
         * NB: This wouldn't work if ATExecSetTableSpace() were allowed to be
         * executed on pg_class or its indexes (the above copy wouldn't contain
-        * the updated pg_class entry), but that's forbidden above.
+        * the updated pg_class entry), but that's forbidden with
+        * CheckRelationTableSpaceMove().
         */
-       rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ? InvalidOid : newTableSpace;
-       rd_rel->relfilenode = newrelfilenode;
-       CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
+       SetRelationTableSpace(rel, newTableSpace, newrelfilenode);
 
        InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
 
-       heap_freetuple(tuple);
-
-       table_close(pg_class, RowExclusiveLock);
-
        relation_close(rel, NoLock);
 
        /* Make sure the reltablespace change is visible */
@@ -13225,56 +13284,25 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpace, LOCKMODE lockmode)
 static void
 ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
 {
-       HeapTuple       tuple;
-       Oid                     oldTableSpace;
-       Relation        pg_class;
-       Form_pg_class rd_rel;
-       Oid                     reloid = RelationGetRelid(rel);
-
        /*
         * Shouldn't be called on relations having storage; these are processed in
         * phase 3.
         */
        Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
 
-       /* Can't allow a non-shared relation in pg_global */
-       if (newTableSpace == GLOBALTABLESPACE_OID)
-               ereport(ERROR,
-                               (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-                                errmsg("only shared relations can be placed in pg_global tablespace")));
-
-       /*
-        * No work if no change in tablespace.
-        */
-       oldTableSpace = rel->rd_rel->reltablespace;
-       if (newTableSpace == oldTableSpace ||
-               (newTableSpace == MyDatabaseTableSpace && oldTableSpace == 0))
+       /* check if relation can be moved to its new tablespace */
+       if (!CheckRelationTableSpaceMove(rel, newTableSpace))
        {
-               InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+               InvokeObjectPostAlterHook(RelationRelationId,
+                                                                 RelationGetRelid(rel),
+                                                                 0);
                return;
        }
 
-       /* Get a modifiable copy of the relation's pg_class row */
-       pg_class = table_open(RelationRelationId, RowExclusiveLock);
-
-       tuple = SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
-       if (!HeapTupleIsValid(tuple))
-               elog(ERROR, "cache lookup failed for relation %u", reloid);
-       rd_rel = (Form_pg_class) GETSTRUCT(tuple);
-
-       /* update the pg_class row */
-       rd_rel->reltablespace = (newTableSpace == MyDatabaseTableSpace) ? InvalidOid : newTableSpace;
-       CatalogTupleUpdate(pg_class, &tuple->t_self, tuple);
-
-       /* Record dependency on tablespace */
-       changeDependencyOnTablespace(RelationRelationId,
-                                                                reloid, rd_rel->reltablespace);
-
-       InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+       /* Update can be done, so change reltablespace */
+       SetRelationTableSpace(rel, newTableSpace, InvalidOid);
 
-       heap_freetuple(tuple);
-
-       table_close(pg_class, RowExclusiveLock);
+       InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
 
        /* Make sure the reltablespace change is visible */
        CommandCounterIncrement();

diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index f57b4034c87c4957a17b19ab76f74146acc155c1..1012cfbcaaeed640e1e6699cede7177e97e6616b 100644 (file)
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -58,6 +58,10 @@ extern void ExecuteTruncateGuts(List *explicit_rels, List *relids, List *relids_
 
 extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass);
 
+extern bool CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId);
+extern void SetRelationTableSpace(Relation rel, Oid newTableSpaceId,
+                                                                 Oid newRelFileNode);
+
 extern ObjectAddress renameatt(RenameStmt *stmt);
 
 extern ObjectAddress renameatt_type(RenameStmt *stmt);