Date: Tue, 26 Jan 2021 15:58:32 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alexey Kondratov <a.kondratov@postgrespro.ru>
Cc: Justin Pryzby <pryzby@telsasoft.com>,
	Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Peter Eisentraut <peter.eisentraut@enterprisedb.com>,
	Masahiko Sawada <masahiko.sawada@2ndquadrant.com>,
	Steve Singer <steve@ssinger.info>,
	pgsql-hackers@lists.postgresql.org,
	Robert Haas <robertmhaas@gmail.com>,
	Alexander Korotkov <a.korotkov@postgrespro.ru>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Jose Luis Tallon <jltallon@adv-solutions.net>
Subject: Re: Allow CLUSTER, VACUUM FULL and REINDEX to change tablespace on
 the fly
Message-ID: <YA+9mAMWYLXJMVPL@paquier.xyz>
References: <03f88f70618ce73e75837b6125a143f7@postgrespro.ru>
 <20210120183439.GA21339@alvherre.pgsql>
 <YAjb2kYBKH07H5Xv@paquier.xyz>
 <bac6739d47f053fe04fbd8cf05283137@postgrespro.ru>
 <e8f209aebf33f02ea118e509a935647a@postgrespro.ru>
 <20210121212651.GY8560@telsasoft.com>
 <944df7cc452fe0c34cf7820da4588d05@postgrespro.ru>
 <YA58Qdgb3a7/yrOH@paquier.xyz>
 <690fa051803c071d213b0d07b5aa9f55@postgrespro.ru>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="J93fYEqPmlh97xzL"
Content-Disposition: inline
In-Reply-To: <690fa051803c071d213b0d07b5aa9f55@postgrespro.ru>
Precedence: bulk


--J93fYEqPmlh97xzL
Content-Type: multipart/mixed; boundary="iMOhYjENVowwyQLo"
Content-Disposition: inline


--iMOhYjENVowwyQLo
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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 f=
ind
> 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
=66rom 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

--iMOhYjENVowwyQLo
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
	filename="v6-0001-Refactor-code-to-detect-and-process-tablespace-mo.patch"
Content-Transfer-Encoding: quoted-printable

=46rom 265631fb5966ae7b454287c489684c8275b7b001 Mon Sep 17 00:00:00 2001
=46rom: Michael Paquier <michael@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/tablec=
mds.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, Lis=
t *relids, List *relids_
=20
 extern void SetRelationHasSubclass(Oid relationId, bool relhassubclass);
=20
+extern bool CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId);
+extern void SetRelationTableSpace(Relation rel, Oid newTableSpaceId,
+								  Oid newRelFileNode);
+
 extern ObjectAddress renameatt(RenameStmt *stmt);
=20
 extern ObjectAddress RenameConstraint(RenameStmt *stmt);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablec=
mds.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 relhass=
ubclass)
 	table_close(relationRelation, RowExclusiveLock);
 }
=20
+/*
+ * 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 =3D RelationGetRelid(rel);
+
+	/*
+	 * No work if no change in tablespace.  Note that MyDatabaseTableSpace
+	 * is stored as 0.
+	 */
+	oldTableSpaceId =3D rel->rd_rel->reltablespace;
+	if (newTableSpaceId =3D=3D oldTableSpaceId ||
+		(newTableSpaceId =3D=3D MyDatabaseTableSpace && oldTableSpaceId =3D=3D 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 =3D=3D 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 =3D RelationGetRelid(rel);
+
+	Assert(CheckRelationTableSpaceMove(rel, newTableSpaceId));
+
+	/* Get a modifiable copy of the relation's pg_class row. */
+	pg_class =3D table_open(RelationRelationId, RowExclusiveLock);
+
+	tuple =3D SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
+	if (!HeapTupleIsValid(tuple))
+		elog(ERROR, "cache lookup failed for relation %u", reloid);
+	rd_rel =3D (Form_pg_class) GETSTRUCT(tuple);
+
+	/* Update the pg_class row. */
+	rd_rel->reltablespace =3D (newTableSpaceId =3D=3D MyDatabaseTableSpace) ?
+		InvalidOid : newTableSpaceId;
+	if (OidIsValid(newRelFileNode))
+		rd_rel->relfilenode =3D 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 =3D NIL;
 	ListCell   *lc;
=20
@@ -13175,45 +13285,15 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpa=
ce, LOCKMODE lockmode)
 	 */
 	rel =3D relation_open(tableOid, lockmode);
=20
-	/*
-	 * No work if no change in tablespace.
-	 */
-	oldTableSpace =3D rel->rd_rel->reltablespace;
-	if (newTableSpace =3D=3D oldTableSpace ||
-		(newTableSpace =3D=3D MyDatabaseTableSpace && oldTableSpace =3D=3D 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;
 	}
=20
-	/*
-	 * 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 =3D=3D 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 =3D 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 newTableSpac=
e, LOCKMODE lockmode)
 		relation_close(toastRel, lockmode);
 	}
=20
-	/* Get a modifiable copy of the relation's pg_class row */
-	pg_class =3D table_open(RelationRelationId, RowExclusiveLock);
-
-	tuple =3D SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(tableOid));
-	if (!HeapTupleIsValid(tuple))
-		elog(ERROR, "cache lookup failed for relation %u", tableOid);
-	rd_rel =3D (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 newTableSpa=
ce, 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 =3D (newTableSpace =3D=3D MyDatabaseTableSpace) ? I=
nvalidOid : newTableSpace;
-	rd_rel->relfilenode =3D 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);
=20
 	RelationAssumeNewRelfilenode(rel);
=20
@@ -13301,58 +13366,25 @@ ATExecSetTableSpace(Oid tableOid, Oid newTableSpa=
ce, LOCKMODE lockmode)
 static void
 ATExecSetTableSpaceNoStorage(Relation rel, Oid newTableSpace)
 {
-	HeapTuple	tuple;
-	Oid			oldTableSpace;
-	Relation	pg_class;
-	Form_pg_class rd_rel;
-	Oid			reloid =3D RelationGetRelid(rel);
-
 	/*
 	 * Shouldn't be called on relations having storage; these are processed in
 	 * phase 3.
 	 */
 	Assert(!RELKIND_HAS_STORAGE(rel->rd_rel->relkind));
=20
-	/* Can't allow a non-shared relation in pg_global */
-	if (newTableSpace =3D=3D 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 =3D rel->rd_rel->reltablespace;
-	if (newTableSpace =3D=3D oldTableSpace ||
-		(newTableSpace =3D=3D MyDatabaseTableSpace && oldTableSpace =3D=3D 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;
 	}
=20
-	/* Get a modifiable copy of the relation's pg_class row */
-	pg_class =3D table_open(RelationRelationId, RowExclusiveLock);
+	/* Update can be done, so change reltablespace */
+	SetRelationTableSpace(rel, newTableSpace, InvalidOid);
=20
-	tuple =3D SearchSysCacheCopy1(RELOID, ObjectIdGetDatum(reloid));
-	if (!HeapTupleIsValid(tuple))
-		elog(ERROR, "cache lookup failed for relation %u", reloid);
-	rd_rel =3D (Form_pg_class) GETSTRUCT(tuple);
-
-	/* update the pg_class row */
-	rd_rel->reltablespace =3D (newTableSpace =3D=3D MyDatabaseTableSpace) ? I=
nvalidOid : 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();
 }
=20
--=20
2.30.0


--iMOhYjENVowwyQLo--

--J93fYEqPmlh97xzL
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmAPvZgACgkQnvQgOdby
QH1TBQ/9E9Pzpi5T0NF3LVfeIl2Xa3nNCYf3v1gij3o8Lq9pP67DrKckw3A0RAEb
IrT3kJpMinpxIa3Z0TbC2jC9Fh3GTcJGArLVbVh2Ulow8/0/yzELAphvZgFcQ/cQ
+OC9o8jO65dEGDEgToSswtSeWAI3KHnAGryXClGBnG11foCexjvAbv5EqKZhRSPb
ez9Oxg/VNEf6T0Aq42Z1bqZBlx+UN4zaUCfjbi5JsY5tfKVBDC+yCiQhcd/z73Y5
qDriNbXzxs0cXA7XhUsLG9ooBHsUTrsoT0fe+MsKl3sq/+gp6yCQ4L6CF2XJn8pu
bz+7jYCyZFt4s9SAwG3lk3Dfdpr+ouW/vNS/eW64kuPE416smFdG+IQSUU1IRzrK
JFnAsmBmwdRpNGfEUg7JkOxk7nju/OJNrAIonUzJTC+Kl5Wh9oeMFio8RY//qpPj
kikYswp9/NB7WRZk2NFNVZBBMIs4txRfaprEFux3aEnU1tkRPo2oVWr5/R2QH9NO
d4Ni/BzNkaqbQCWwUJFD5OLDt9t1YgrXIEyXwQqqJvGthK0/u+OlpeNgkqWX82IP
Gl8wRsou7i4bHR0agxVjmKmLEQc2c71gZP2wfZX44CVMQ94oBNQT0UW2/FKiOGbp
qdl3je4toQLjsbtagGEgu5v95lXcEbL7+jLfeIWXhhlHXsK2hN4=
=xqIr
-----END PGP SIGNATURE-----

--J93fYEqPmlh97xzL--