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: Mon, 12 Jan 2026 17:27:30 -0500
Message-ID: <CAAKRu_ZJXU8UoN+T1AjgT_qnRNoOWfw4O-C+6FYryvKj9DHO5A@mail.gmail.com> (raw)
In-Reply-To: <kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>
References: <3nce7i72ayzkunai6mkz24ckbxk74jodz4ua2chcdrwppxlxcd@w6x5kfkjrkru>
	<lneuyxqxamqoayd2ntau3lqjblzdckw6tjgeu4574ezwh4tzlg@noioxkquezdw>
	<[email protected]>
	<q3ybyaiownp4ivt6xfy55s5llvtq6vqsd4zbt5kn6hacxdcpp4@2ngd4h4bev5d>
	<dsbacri4xldlobl3jbqqlksefsyrwj7lzljbohxsbqjus3z4o7@otb6rge4w7az>
	<[email protected]>
	<5dwlfu2jyzkyf3nrlzxxblxctb6xio5es73ptgsahjnmfu5miu@772rc764hfhi>
	<ossv2eistssmubfsir6xjll76tynvxv5lup4zkrfzjkud7fycw@rf5vii6l6cha>
	<4csodkvvfbfloxxjlkgsnl2lgfv2mtzdl7phqzd4jxjadxm4o5@usw7feyb5bzf>
	<CALdSSPgyc7VuMLUZ8J7v2G-PebBa_vEi+mp-cLkcOwNycB56Hw@mail.gmail.com>
	<kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>

On Mon, Jan 12, 2026 at 12:45 PM Andres Freund <[email protected]> wrote:
>
> Also working on doing comment polishing of the later patches, found a few
> things, but not quite enough to be worth reposting yet.

I looked at 0004 and 0005 and re-looked at 0006 - 0007.

For 0004, I think you should clarify the commit message a bit. I had
trouble understanding when WAKE_IN_PROGRESS is set. So, before
RELEASE_OK was set all the time except when a process woke up and
hadn't run yet. Now, WAKE_IN_PROGRESS is only set when a process is
woken up but hasn't run yet. Personally, I just needed a bit more
specificity (maybe even a bit more formality and grammatically
correctness) from the commit message to get it.

I agree that separating 0005 is helpful.

0007 looks basically fine to me. I'd comb through it with an AI tool
to catch a few nits I saw like an outdated reference to RELEASE_OK and
a missing word in the commit message, etc.

Otherwise, I mostly looked to see if the wakeup semantics seemed right
and if anything jumped out at me while skimming (i.e. I didn't go
through every line with a fine-toothed comb).

The two things I came up with were:

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));
+    }
 }

 is it related to your comment in the commit message

2) Error recovery for content locks is implemented as part of the
already existing private-refcount tracking mechanism in combination
with resowners?

As for your FIXMEs,
+    /*
+     * FIXME: This is reusing the lwlock fields. That's not a correctness
+     * issue, a backend can't wait for both an lwlock and a buffer content
+     * lock at the same time. However, it seems pretty ugly, particularly
+     * given that the field names have an lw* prefix. But duplicating the
+     * fields also seems somewhat superfluous.
+     */

personally I can live with reusing the lwlock fields now that it's
fairly well documented.

+    /* XXX: combine with fetch_and above? */
+    UnlockBufHdr(buf_hdr);

Are you thinking about adding a helper that stops waiting and unlocks?

> Perhaps move the locking code into a buffer_locking.h or such? Needs to be inline functions for efficiency unfortunately.

So you mean put all of the static buffer locking functions you added
to bufmgr.c inline into a header file?

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. For example, I find it really annoying that
the helper function prototypes for res owner and ref count related
functions are grouped before their implementations and then below that
there is another seemingly arbitrary group of prototypes and then
their implementations. Like, what is the logic there?

- 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_ZJXU8UoN+T1AjgT_qnRNoOWfw4O-C+6FYryvKj9DHO5A@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