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. 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. -- Michael
From 265631fb5966ae7b454287c489684c8275b7b001 Mon Sep 17 00:00:00 2001
From: Michael Paquier <mich...@paquier.xyz>
Date: Tue, 26 Jan 2021 15:53:06 +0900
Subject: [PATCH v6] Refactor code to detect and process tablespace moves
---
src/include/commands/tablecmds.h | 4 +
src/backend/commands/tablecmds.c | 222 ++++++++++++++++++-------------
2 files changed, 131 insertions(+), 95 deletions(-)
diff --git a/src/include/commands/tablecmds.h b/src/include/commands/tablecmds.h
index 08c463d3c4..b3d30acc35 100644
--- a/src/include/commands/tablecmds.h
+++ b/src/include/commands/tablecmds.h
@@ -61,6 +61,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 RenameConstraint(RenameStmt *stmt);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 8687e9a97c..1f88eebabd 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3037,6 +3037,120 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
table_close(relationRelation, RowExclusiveLock);
}
+/*
+ * CheckRelationTableSpaceMove
+ * Check if relation can be moved to new tablespace.
+ *
+ * NOTE: caller must be holding an appropriate lock on the relation.
+ * ShareUpdateExclusiveLock is sufficient to prevent concurrent schema
+ * changes.
+ *
+ * Returns true if the relation can be moved to the new tablespace;
+ * false otherwise.
+ */
+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;
+ }
+
+ /*
+ * 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 file node. If newrelfilenode is InvalidOid,
+ * this field is not updated.
+ *
+ * NOTE: caller must be holding an appropriate lock on the relation.
+ * ShareUpdateExclusiveLock is sufficient.
+ *
+ * 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);
+
+ InvokeObjectPostAlterHook(RelationRelationId, reloid, 0);
+
+ heap_freetuple(tuple);
+ table_close(pg_class, RowExclusiveLock);
+}
+
/*
* renameatt_check - basic sanity checks before attribute rename
*/
@@ -13160,13 +13274,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;
@@ -13175,45 +13285,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))
@@ -13224,14 +13304,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.
@@ -13262,17 +13334,10 @@ 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);
-
- InvokeObjectPostAlterHook(RelationRelationId, RelationGetRelid(rel), 0);
-
- heap_freetuple(tuple);
-
- table_close(pg_class, RowExclusiveLock);
+ SetRelationTableSpace(rel, newTableSpace, newrelfilenode);
RelationAssumeNewRelfilenode(rel);
@@ -13301,58 +13366,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);
+ /* Update can be done, so change reltablespace */
+ SetRelationTableSpace(rel, newTableSpace, InvalidOid);
- 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);
-
- heap_freetuple(tuple);
-
- table_close(pg_class, RowExclusiveLock);
-
- /* Make sure the reltablespace change is visible */
+ /* Make the change visible */
CommandCounterIncrement();
}
--
2.30.0
signature.asc
Description: PGP signature
