MIME-Version: 1.0
References: <5dwlfu2jyzkyf3nrlzxxblxctb6xio5es73ptgsahjnmfu5miu@772rc764hfhi>
 <ossv2eistssmubfsir6xjll76tynvxv5lup4zkrfzjkud7fycw@rf5vii6l6cha>
 <4csodkvvfbfloxxjlkgsnl2lgfv2mtzdl7phqzd4jxjadxm4o5@usw7feyb5bzf>
 <CALdSSPgyc7VuMLUZ8J7v2G-PebBa_vEi+mp-cLkcOwNycB56Hw@mail.gmail.com>
 <kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>
 <jtg5cu4n6h5lib3kzx66ju4yhh6kmviaud7oq6dtut6c4q4rdi@xwsfoagt3c2b>
 <k5j77f3q6ztihnjnx2nqxzyor6fbj2qxcbhzuxhkh2yy63jyfg@p72phigar3n4>
 <5ubipyssiju5twkb7zgqwdr7q2vhpkpmuelxfpanetlk6ofnop@hvxb4g2amb2d>
 <68e89de8-5f6c-4eaf-a800-e16a5e487667@iki.fi>
 <20260215195239.ce.noahmisch@microsoft.com>
 <mheeefrtikvgjnjsenocvo3afj7vlpr5rljkzevssxs77n2zdt@sugqtjhryydo>
In-Reply-To: <mheeefrtikvgjnjsenocvo3afj7vlpr5rljkzevssxs77n2zdt@sugqtjhryydo>
From: Melanie Plageman <melanieplageman@gmail.com>
Date: Wed, 25 Mar 2026 17:34:33 -0400
Message-ID: 
 <CAAKRu_Z=xrBWhAB5wRG8x-gnjNa7ZnW-sSsEbBfR1C8gwuTD8g@mail.gmail.com>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
To: Andres Freund <andres@anarazel.de>
Cc: Noah Misch <noah@leadboat.com>, Heikki Linnakangas <hlinnaka@iki.fi>,
	Kirill Reshke <reshkekirill@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
 pgsql-hackers@postgresql.org,
	Thomas Munro <thomas.munro@gmail.com>, Robert Haas <robertmhaas@gmail.com>,
	Michael Paquier <michael.paquier@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAAKRu_Z%3DxrBWhAB5wRG8x-gnjNa7ZnW-sSsEbBfR1C8gwuTD8g%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 11, 2026 at 6:40=E2=80=AFPM Andres Freund <andres@anarazel.de> =
wrote:
>
> I pushed this and many of the later patches in the series.  Here are upda=
ted
> versions of the remaining changes.  The last two previously were one comm=
it
> with "WIP" in the title. The first one has, I think, not had a lot of rev=
iew -
> but it's also not a complicated change.

0001 looks good except for the comment above PageSetChecksum() that
says it is only for shared buffers and a stray reference to the
no-longer-present bufToWrite variable in a comment around line 4490 in
bufmgr.c

0002
diff --git a/src/backend/access/nbtree/nbtpage.c
b/src/backend/access/nbtree/nbtpage.c
index cc9c45dc40c..ad700e590e8 100644
--- a/src/backend/access/nbtree/nbtpage.c
+++ b/src/backend/access/nbtree/nbtpage.c
@@ -1011,24 +1011,48 @@ _bt_relandgetbuf(Relation rel, Buffer obuf,
BlockNumber blkno, int access)
    Assert(BlockNumberIsValid(blkno));
    if (BufferIsValid(obuf))
-       _bt_unlockbuf(rel, obuf);
-   buf =3D ReleaseAndReadBuffer(obuf, rel, blkno);
-   _bt_lockbuf(rel, buf, access);
+   {
+       if (BufferGetBlockNumber(obuf) =3D=3D blkno)
+       {
+           /* trade in old lock mode for new lock */
+           _bt_unlockbuf(rel, obuf);
+           buf =3D obuf;
+       }
+       else
+       {
+           /* release lock and pin at once, that's a bit more efficient */
+           _bt_relbuf(rel, obuf);
+           buf =3D ReadBuffer(rel, blkno);
+       }
+   }
+   else
+       buf =3D ReadBuffer(rel, blkno);

Not related to this patch, but why do we unlock and relock it when
obuf has the block we need? Couldn't we pass lock mode and then just
do nothing if it is the right lockmode?

Setting that aside, I presume we don't need to check the fork and
relfilelocator (as ReleaseAndReadBuffer() did) because this code knows
it will be the same?

Anyway, LGTM.

0003
AFAICT, this does what you claim. I don't really know what else to
look when reviewing it, if I'm being honest. As such, I diligently fed
it through AI which suggested you may have lost a
        VALGRIND_MAKE_MEM_NOACCESS(BufHdrGetBlock(buf), BLCKSZ);
which sounds right to me and like something you should fix.

Also, I'd say this comment
+   /*
+    * Now okay to allow cancel/die interrupts again, were held when the lo=
ck
+    * was acquired.
+    */

needs a "which" after the comma to read smoothly.

- Melanie