Re: Buffer locking is special (hints, checksums, AIO writes)

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Melanie Plageman <[email protected]>
To: Andres Freund <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: [email protected]
Cc: Thomas Munro <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Date: Tue, 13 Jan 2026 09:59:12 -0500
Message-ID: <CAAKRu_YPPNnYaEtk-YMPV+F=5W70qfPkHNekddLOBZmxfT2ADw@mail.gmail.com> (raw)
In-Reply-To: <ql4dlxwwj5u2ia5enmcw3jagj4hr224me4uoyfsxo2nbgtyjnw@42bqyxxzfyat>
References: <[email protected]>
	<q3ybyaiownp4ivt6xfy55s5llvtq6vqsd4zbt5kn6hacxdcpp4@2ngd4h4bev5d>
	<dsbacri4xldlobl3jbqqlksefsyrwj7lzljbohxsbqjus3z4o7@otb6rge4w7az>
	<[email protected]>
	<5dwlfu2jyzkyf3nrlzxxblxctb6xio5es73ptgsahjnmfu5miu@772rc764hfhi>
	<ossv2eistssmubfsir6xjll76tynvxv5lup4zkrfzjkud7fycw@rf5vii6l6cha>
	<4csodkvvfbfloxxjlkgsnl2lgfv2mtzdl7phqzd4jxjadxm4o5@usw7feyb5bzf>
	<CALdSSPgyc7VuMLUZ8J7v2G-PebBa_vEi+mp-cLkcOwNycB56Hw@mail.gmail.com>
	<kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>
	<CAAKRu_ZJXU8UoN+T1AjgT_qnRNoOWfw4O-C+6FYryvKj9DHO5A@mail.gmail.com>
	<ql4dlxwwj5u2ia5enmcw3jagj4hr224me4uoyfsxo2nbgtyjnw@42bqyxxzfyat>

On Mon, Jan 12, 2026 at 6:22 PM Andres Freund <[email protected]> wrote:
>
> Is this better?
>     lwlock: Invert meaning of LW_FLAG_RELEASE_OK
>
>     Previously, a flag was set to indicate that a lock release should wake up
>     waiters. Since waking waiters is the default behavior in the majority of
>     cases, this logic has been inverted. The new LW_FLAG_WAKE_IN_PROGRESS flag is
>     now set iff wakeups are explicitly inhibited.

I think what you have would work for most people. The key thing for me
is that the wakeups are inhibited _because_ someone else is already
awake. So, you don't have to wake anyone up when you release the lock
because there is already someone awake. Having you explain that
off-list was necessary for me to bridge the gap between RELEASE_NOT_OK
and WAKE_IN_PROGRESS. And I do agree that WAKE_IN_PROGRESS is more
descriptive of when the flag is actually set. RELEASE_NOT_OK doesn't
explain the state or who/when it should be set.

> > I wondered why this was needed (i.e. why it wasn't needed before)
>
> > @@ -6688,7 +7428,25 @@ ResOwnerReleaseBufferPin(Datum res)
> >      if (BufferIsLocal(buffer))
> >          UnpinLocalBufferNoOwner(buffer);
> >      else
> > +    {
> > +        PrivateRefCountEntry *ref;
> > +
> > +        ref = GetPrivateRefCountEntry(buffer, false);
> > +
> > +        /*
> > +         * If the buffer was locked at the time of the resowner release,
> > +         * release the lock now. This should only happen after errors.
> > +         */
> > +        if (ref->data.lockmode != BUFFER_LOCK_UNLOCK)
> > +        {
> > +            BufferDesc *buf = GetBufferDescriptor(buffer - 1);
> > +
> > +            HOLD_INTERRUPTS();    /* match the upcoming RESUME_INTERRUPTS */
> > +            BufferLockUnlock(buffer, buf);
> > +        }
> > +
> >          UnpinBufferNoOwner(GetBufferDescriptor(buffer - 1));
> > +    }
> >  }
>
> It's needed because previously content locks were released as part of the
> LWLockReleaseAll() that are sprinkled across various error recovery paths. Now
> that content locks aren't implemented via lwlocks anymore, something new is needed.

And all those LWLockReleaseAll()s are still needed because we might
hold other LWLocks even though we won't hold them for buffer content
access?

> > +    /* XXX: combine with fetch_and above? */
> > +    UnlockBufHdr(buf_hdr);
> >
> > Are you thinking about adding a helper that stops waiting and unlocks?
>
> I'm not sure what you mean by that? Just whether I plan to implement the
> FIXME?

I was trying to figure out why you left it as a FIXME and didn't just
do it or not do it. I thought maybe it was because you weren't sure if
you wanted to add another helper in addition to UnlockBufHdr().

> > bufmgr.c is super long anyway, so it's not like making it separate
> > makes the file manageable. On the other hand, it's probably better to
> > not keep making it worse.
>
> Yea. OTOH I don't know if a header that's just included by one file is really
> an improvement :/

Yea, I suppose that is a bit odd. Though it could be a pattern you
start for organizing gigantic files. I'm overall a +0.7 unless you
explain some other downsides than oddity.

- Melanie

view thread (35+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
  In-Reply-To: <CAAKRu_YPPNnYaEtk-YMPV+F=5W70qfPkHNekddLOBZmxfT2ADw@mail.gmail.com>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox