pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Refactor code in tablecmds.c to check and process tablespace mov
5+ messages / 2 participants
[nested] [flat]
Commits from this thread:
2021-01-29 24843297a9 Adjust comments of CheckRelationTableSpaceMove() and SetRelationTableSpace()
* pgsql: Refactor code in tablecmds.c to check and process tablespace mov
@ 2021-01-27 02:55 Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2021-01-27 02:55 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Refactor code in tablecmds.c to check and process tablespace moves
Two code paths of tablecmds.c (for relations with storage and without
storage) use the same logic to check if the move of a relation to a
new tablespace is allowed or not and to update pg_class.reltablespace
and pg_class.relfilenode.
A potential TABLESPACE clause for REINDEX, CLUSTER and VACUUM FULL needs
similar checks to make sure that nothing is moved around in illegal ways
(no mapped relations, shared relations only in pg_global, no move of
temp tables owned by other backends).
This reorganizes the existing code of ALTER TABLE so as all this logic
is controlled by two new routines that can be reused for the other
commands able to move relations across tablespaces, limiting the number
of code paths in need of the same protections. This also removes some
code that was duplicated for tables with and without storage for ALTER
TABLE.
Author: Alexey Kondratov, Michael Paquier
Discussion: https://postgr.es/m/YA+9mAMWYLXJMVPL@paquier.xyz
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/4c9c359d38ff1e2de388eedd860785be6a49201c
Modified Files
--------------
src/backend/commands/tablecmds.c | 211 ++++++++++++++++++++++-----------------
src/include/commands/tablecmds.h | 4 +
2 files changed, 124 insertions(+), 91 deletions(-)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Refactor code in tablecmds.c to check and process tablespace mov
@ 2021-01-27 14:07 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Alvaro Herrera @ 2021-01-27 14:07 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-committers@lists.postgresql.org
On 2021-Jan-27, Michael Paquier wrote:
> Refactor code in tablecmds.c to check and process tablespace moves
Thanks, looks good. Small comment: CheckRelationTableSpaceMove is
documented as
+ * Returns true if the relation can be moved to the new tablespace;
+ * false otherwise.
but in reality it returns false if the relation does not *need* to be
moved because the tablespace is the same as before. In the cases where
it "cannot" be moved, what it does is raise an error.
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."
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.
--
Álvaro Herrera 39°49'30"S 73°17'W
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Refactor code in tablecmds.c to check and process tablespace mov
@ 2021-01-28 06:29 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2021-01-28 06:29 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-committers@lists.postgresql.org
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) download
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()
* Re: pgsql: Refactor code in tablecmds.c to check and process tablespace mov
@ 2021-01-28 13:56 Alvaro Herrera <alvherre@alvh.no-ip.org>
parent: Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 5+ messages in thread
From: Alvaro Herrera @ 2021-01-28 13:56 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-committers@lists.postgresql.org
On 2021-Jan-28, Michael Paquier wrote:
> So, for now, I would just do the attached to address your
> suggestions. Is that fine for you?
Yep, seems OK to me, assuming that, as you say, you'll update the
comments again when they are used to handle the concurrent cases.
Thanks!
--
Álvaro Herrera Valdivia, Chile
"There was no reply" (Kernel Traffic)
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgsql: Refactor code in tablecmds.c to check and process tablespace mov
@ 2021-01-29 05:05 Michael Paquier <michael@paquier.xyz>
parent: Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 5+ messages in thread
From: Michael Paquier @ 2021-01-29 05:05 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: pgsql-committers@lists.postgresql.org
On Thu, Jan 28, 2021 at 10:56:29AM -0300, Alvaro Herrera wrote:
> Yep, seems OK to me, assuming that, as you say, you'll update the
> comments again when they are used to handle the concurrent cases.
That's the idea. I need first to look in depth at what's proposed on
the other thread for REINDEX, but I see no reason why the case of
REINDEX CONCURRENTLY would not be able to use this stuff. Applied
this part for now. Thanks.
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2021-01-29 05:05 UTC | newest]
Thread overview: 5+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-01-27 02:55 pgsql: Refactor code in tablecmds.c to check and process tablespace mov Michael Paquier <michael@paquier.xyz>
2021-01-27 14:07 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-01-28 06:29 ` Michael Paquier <michael@paquier.xyz>
2021-01-28 13:56 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2021-01-29 05:05 ` Michael Paquier <michael@paquier.xyz>
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