From dc845383cd12dcbb09b468b2eea94bf549528a4b Mon Sep 17 00:00:00 2001 From: Noah Misch Date: Tue, 24 Sep 2024 15:25:24 -0700 Subject: [PATCH] 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 --- src/backend/commands/tablecmds.c | 210 +++++++++++++++++-------------- src/include/commands/tablecmds.h | 4 + 2 files changed, 123 insertions(+), 91 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 78d529c22c0..ae21e995182 100644 --- 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 f57b4034c87..1012cfbcaae 100644 --- 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); -- 2.39.5