Horton

Thread

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Add assertions that we hold some relevant lock during relation open.

  1. Surround CheckRelation[Oid]LockedByMe() with USE_ASSERT_CHECKING

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-01T06:42:46Z

    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

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

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-07-01T07:05:34Z

    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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-01T07:52:59Z

    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

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

    Michael Paquier <michael@paquier.xyz> — 2024-07-01T08:01:46Z

    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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-01T09:41:31Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2024-07-01T14:21:35Z

    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

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

    Bertrand Drouvot <bertranddrouvot.pg@gmail.com> — 2024-07-01T14:38:44Z

    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