Thread: 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
Attachment
Hi, On Mon, Jul 1, 2024 at 12:12 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> 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
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 > <bertranddrouvot.pg@gmail.com> 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
On Mon, Jul 01, 2024 at 06:42:46AM +0000, 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
Attachment
Hi, On Mon, Jul 01, 2024 at 05:01:46PM +0900, Michael Paquier wrote: > On Mon, Jul 01, 2024 at 06:42:46AM +0000, 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
Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Jul 01, 2024 at 06:42:46AM +0000, 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
Hi, On Mon, Jul 01, 2024 at 10:21:35AM -0400, Tom Lane wrote: > Michael Paquier <michael@paquier.xyz> writes: > > On Mon, Jul 01, 2024 at 06:42:46AM +0000, 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