Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
Hi, On Mon, Jul 01, 2024 at 10:21:35AM -0400, Tom Lane wrote: > Michael Paquier writes: > > On Mon, Jul 01, 2024 at 06:42:46AM +, Bertrand Drouvot wrote: > >> I think it would make sense to declare / define those functions only for > >> assert enabled build: please find attached a tiny patch doing so. > > > Not convinced that's a good idea. What about out-of-core code that > > may use these routines for runtime checks in non-assert builds? > > Yeah. Also, I believe it's possible for an extension that's been > built with assertions enabled to be used with a core server that > wasn't. This is why, for example, ExceptionalCondition() is not > ifdef'd away in a non-assert build. Even if you think there's > no use for CheckRelation[Oid]LockedByMe except in assertions, > it'd still be plenty reasonable for an extension to call them > in assertions. Yeah good point, thanks for the feedback! I've withdrawn the CF entry. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
Michael Paquier writes: > On Mon, Jul 01, 2024 at 06:42:46AM +, Bertrand Drouvot wrote: >> I think it would make sense to declare / define those functions only for >> assert enabled build: please find attached a tiny patch doing so. > Not convinced that's a good idea. What about out-of-core code that > may use these routines for runtime checks in non-assert builds? Yeah. Also, I believe it's possible for an extension that's been built with assertions enabled to be used with a core server that wasn't. This is why, for example, ExceptionalCondition() is not ifdef'd away in a non-assert build. Even if you think there's no use for CheckRelation[Oid]LockedByMe except in assertions, it'd still be plenty reasonable for an extension to call them in assertions. regards, tom lane
Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
Hi, On Mon, Jul 01, 2024 at 05:01:46PM +0900, Michael Paquier wrote: > On Mon, Jul 01, 2024 at 06:42:46AM +, Bertrand Drouvot wrote: > > While working on a rebase for [1] due to 0cecc908e97, I noticed that > > CheckRelationLockedByMe() and CheckRelationOidLockedByMe() are used only in > > assertions. > > > > I think it would make sense to declare / define those functions only for > > assert enabled build: please find attached a tiny patch doing so. > > > > Thoughts? > > Not convinced that's a good idea. What about out-of-core code that > may use these routines for runtime checks in non-assert builds? Thanks for the feedback. Yeah that could be an issue for CheckRelationLockedByMe() (CheckRelationOidLockedByMe() is too recent to be a concern). Having said that 1. out of core could want to use CheckRelationOidLockedByMe() ( probably if it was already using CheckRelationLockedByMe()) and 2. I just submitted a rebase for [1] in which I thought that using CheckRelationOidLockedByMe() would be a good idea. So I think that we can get rid of this proposal. [1]: https://www.postgresql.org/message-id/ZoJ5RVtMziIa3TQp%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
On Mon, Jul 01, 2024 at 06:42:46AM +, Bertrand Drouvot wrote: > While working on a rebase for [1] due to 0cecc908e97, I noticed that > CheckRelationLockedByMe() and CheckRelationOidLockedByMe() are used only in > assertions. > > I think it would make sense to declare / define those functions only for > assert enabled build: please find attached a tiny patch doing so. > > Thoughts? Not convinced that's a good idea. What about out-of-core code that may use these routines for runtime checks in non-assert builds? -- Michael signature.asc Description: PGP signature
Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
Hi, On Mon, Jul 01, 2024 at 12:35:34PM +0530, Bharath Rupireddy wrote: > Hi, > > On Mon, Jul 1, 2024 at 12:12 PM Bertrand Drouvot > wrote: > > > > Hi hackers, > > > > While working on a rebase for [1] due to 0cecc908e97, I noticed that > > CheckRelationLockedByMe() and CheckRelationOidLockedByMe() are used only in > > assertions. > > > > I think it would make sense to declare / define those functions only for > > assert enabled build: please find attached a tiny patch doing so. > > > > Thoughts? > > If turning the CheckRelationXXXLocked() compile for non-assert builds, > why not do the same for LWLockHeldByMe, LWLockAnyHeldByMe and > LWLockHeldByMeInMode that are debug-only and being used in asserts? > While it might reduce the compiled binary size a bit for release > builds, we may have to be cautious about external or out of core > modules using them. Thanks for the feedback. CheckRelationOidLockedByMe() is new (as it has been added in 0cecc908e97. While its counterpart CheckRelationLockedByMe() has been added since a few years (2018) in commit b04aeb0a053, I thought it would make sense to surround both of them. While it's true that we could also surround LWLockHeldByMe() (added in e6cba71503f , 2004 and signature change in ea9df812d85, 2014), LWLockAnyHeldByMe() (added in eed959a457e, 2022) and LWLockHeldByMeInMode() (added in 016abf1fb83, 2016), I'm not sure we should (due to their "age" and as you said we have to be cautious about out of core modules / extensions that may use them). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
Hi, On Mon, Jul 1, 2024 at 12:12 PM Bertrand Drouvot wrote: > > Hi hackers, > > While working on a rebase for [1] due to 0cecc908e97, I noticed that > CheckRelationLockedByMe() and CheckRelationOidLockedByMe() are used only in > assertions. > > I think it would make sense to declare / define those functions only for > assert enabled build: please find attached a tiny patch doing so. > > Thoughts? If turning the CheckRelationXXXLocked() compile for non-assert builds, why not do the same for LWLockHeldByMe, LWLockAnyHeldByMe and LWLockHeldByMeInMode that are debug-only and being used in asserts? While it might reduce the compiled binary size a bit for release builds, we may have to be cautious about external or out of core modules using them. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING
Hi hackers, While working on a rebase for [1] due to 0cecc908e97, I noticed that CheckRelationLockedByMe() and CheckRelationOidLockedByMe() are used only in assertions. I think it would make sense to declare / define those functions only for assert enabled build: please find attached a tiny patch doing so. Thoughts? [1]: https://www.postgresql.org/message-id/flat/ZiYjn0eVc7pxVY45%40ip-10-97-1-34.eu-west-3.compute.internal Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com >From bee765581b6b356a6c9c2e4407243f1622b8e84a Mon Sep 17 00:00:00 2001 From: Bertrand Drouvot Date: Mon, 1 Jul 2024 05:38:30 + Subject: [PATCH v1] Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING As CheckRelationLockedByMe() and CheckRelationOidLockedByMe() are used only in assertions, declare / define those functions only for assert enabled build. --- src/backend/storage/lmgr/lmgr.c | 2 ++ src/include/storage/lmgr.h | 2 ++ 2 files changed, 4 insertions(+) 50.0% src/backend/storage/lmgr/ 50.0% src/include/storage/ diff --git a/src/backend/storage/lmgr/lmgr.c b/src/backend/storage/lmgr/lmgr.c index 094522acb41..234cab4f7af 100644 --- a/src/backend/storage/lmgr/lmgr.c +++ b/src/backend/storage/lmgr/lmgr.c @@ -318,6 +318,7 @@ UnlockRelation(Relation relation, LOCKMODE lockmode) LockRelease(&tag, lockmode, false); } +#ifdef USE_ASSERT_CHECKING /* * CheckRelationLockedByMe * @@ -352,6 +353,7 @@ CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode, bool orstronger) return LockHeldByMe(&tag, lockmode, orstronger); } +#endif /* * LockHasWaitersRelation diff --git a/src/include/storage/lmgr.h b/src/include/storage/lmgr.h index ce15125ac3b..3d5f989ae80 100644 --- a/src/include/storage/lmgr.h +++ b/src/include/storage/lmgr.h @@ -46,10 +46,12 @@ extern void UnlockRelationOid(Oid relid, LOCKMODE lockmode); extern void LockRelation(Relation relation, LOCKMODE lockmode); extern bool ConditionalLockRelation(Relation relation, LOCKMODE lockmode); extern void UnlockRelation(Relation relation, LOCKMODE lockmode); +#ifdef USE_ASSERT_CHECKING extern bool CheckRelationLockedByMe(Relation relation, LOCKMODE lockmode, bool orstronger); extern bool CheckRelationOidLockedByMe(Oid relid, LOCKMODE lockmode, bool orstronger); +#endif extern bool LockHasWaitersRelation(Relation relation, LOCKMODE lockmode); extern void LockRelationIdForSession(LockRelId *relid, LOCKMODE lockmode); -- 2.34.1
