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
Date: Thu, 28 Jan 2021 15:29:40 +0900
Message-ID: <YBJZ1IE4ALxcLfGJ@paquier.xyz> (raw)
In-Reply-To: <20210127140741.GA14174@alvherre.pgsql>
References: <E1l4az0-0000uQ-Mz@gemulon.postgresql.org>
<20210127140741.GA14174@alvherre.pgsql>
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:
>
> "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
Attachments:tbspace-move-comments.patch (text/x-diff, 1.6K) downloaddiff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.cindex 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()
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: michael@paquier.xyz, alvherre@alvh.no-ip.org, pgsql-committers@lists.postgresql.org
Subject: Re: pgsql: Refactor code in tablecmds.c to check and process tablespace mov
In-Reply-To: <YBJZ1IE4ALxcLfGJ@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox