Thread: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

From
Bertrand Drouvot
Date:
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

Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

From
Bharath Rupireddy
Date:
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



Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

From
Bertrand Drouvot
Date:
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



Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

From
Michael Paquier
Date:
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

Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

From
Bertrand Drouvot
Date:
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



Re: Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

From
Bertrand Drouvot
Date:
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