Date: Thu, 28 Jan 2021 15:29:40 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Refactor code in tablecmds.c to check and process
 tablespace mov
Message-ID: <YBJZ1IE4ALxcLfGJ@paquier.xyz>
References: <E1l4az0-0000uQ-Mz@gemulon.postgresql.org>
 <20210127140741.GA14174@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="4oq1gEGRK4IM0Wsn"
Content-Disposition: inline
In-Reply-To: <20210127140741.GA14174@alvherre.pgsql>
Precedence: bulk


--4oq1gEGRK4IM0Wsn
Content-Type: multipart/mixed; boundary="9AVxgdRpGQ8FRUJy"
Content-Disposition: inline


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

On Wed, Jan 27, 2021 at 11:07:41AM -0300, Alvaro Herrera wrote:
> Thanks, looks good.  Small comment: CheckRelationTableSpaceMove is
> documented as

  * newTableSpaceId is the new tablespace for the relation, and
- * newRelFileNode its new filenode.  If newrelfilenode is InvalidOid,
+ * newRelFileNode its new filenode.  If newRelFileNode is InvalidOid,
  * this field is not updated.
Looks like I have put an incorrect variable name in one of the new
comments as well.

> Maybe this needs is just be documented more clearly:
>=20
> "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."

Good idea.

> For cases of relation with storage, I find it suspicious that the
> functions are documented to be fine with just ShareUpdateExclusiveLock.
> I think it'd be safer for the comment to explicitly indicate that for
> relations with storage, lock should be AEL.

Yeah, it is true that ShareUpdateExclusiveLock may not be completely
safe as it depends on the context where the operation is done.  For
REINDEX CONCURRENTLY, it would actually be logically fine even if an
AEL is not hold on the relation thanks to the intermediate waits.  You
are right that switching to an AEL in this part makes sense as of
HEAD.  However, for REINDEX CONCURRENTLY we will also make use of
these routines, but maybe we could just add an extra note the comment
block of this stuff once we begin to use it?

So, for now, I would just do the attached to address your
suggestions.  Is that fine for you?
--
Michael

--9AVxgdRpGQ8FRUJy
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment; filename="tbspace-move-comments.patch"

diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 36747e7277..420991e315 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -3041,11 +3041,11 @@ SetRelationHasSubclass(Oid relationId, bool relhassubclass)
  * CheckRelationTableSpaceMove
  *		Check if relation can be moved to new tablespace.
  *
- * NOTE: Caller must be holding an appropriate lock on the relation.
- * ShareUpdateExclusiveLock is sufficient.
+ * NOTE: The caller must hold AccessExclusiveLock on the relation.
  *
- * Returns true if the relation can be moved to the new tablespace;
- * false otherwise.
+ * 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)
@@ -3094,11 +3094,10 @@ CheckRelationTableSpaceMove(Relation rel, Oid newTableSpaceId)
  *		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,
+ * newRelFileNode its new filenode.  If newRelFileNode is InvalidOid,
  * this field is not updated.
  *
- * NOTE: Caller must be holding an appropriate lock on the relation.
- * ShareUpdateExclusiveLock is sufficient.
+ * 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()

--9AVxgdRpGQ8FRUJy--

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

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

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAmASWdQACgkQnvQgOdby
QH2YCg//YeflmnmmSz1h8Y+stYe5Si1kN1AfO5ChK6cNebqHvtyFU6PSr+DGf/sM
dNY6QLAWxOL2Rz/mVC5TKj/XRii8NbGDYbmUhNH+uYG4GnILL9xxYZ6rQyCQtuO/
dfhZvzSpxXu5x+TTwZDoCa77UN/WmSTZl0BB4yxZ1yfHhtxW4ExiodqCrDbqxBet
OM5AinN8bMY6GCLj9MtS/bydJIkVbPP/Za++oW1SIclUwCJ9O8Nhtb0qNEjeZGDt
1/lkJZkX2HUw7q2sYIMw74mtv57jzASfQHN5RFdLFkmPrenEjh7Xs5AOyjJnDmXE
i1B94uzmruJCIdXuEdR9tJWHnV3V/VHlsktdl+Q0oFsaGOuybkMFcJhrGWOzTVnm
Y0pe6CUt2L7tOnpq8kZMKrr4KVgVfW8uWRCkFCew1wxDmsrotOJ1Y+U2g3ofT+ZS
8KdyJnX6OR7bfjpS/ZqNRBj42VzwUBfnE0QGyn6A80CVXhWHRe/Az/nTRqfEBwJD
evtAjmoLXoKiU2/a5OL4X3wBEFf2xMlwpcJypnss5ThMC6rTJRgpWKUx7iuQDWa8
XYXCRIml61q2Js9uOCn8q4Tv4ggtAkM/YotPb/IY8ae9wZ8o4xI1lfD8XwXrkB+5
vzjp21l0cLNHUQXJB3PEXgznzCEm+t2nQTikDr2mx9M6EXaiRFQ=
=54O+
-----END PGP SIGNATURE-----

--4oq1gEGRK4IM0Wsn--