public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Melanie Plageman <[email protected]>
Cc: Noah Misch <[email protected]>
Cc: Heikki Linnakangas <[email protected]>
Cc: Kirill Reshke <[email protected]>
Cc: Matthias van de Meent <[email protected]>
Cc: [email protected]
Cc: Thomas Munro <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Michael Paquier <[email protected]>
Subject: Re: Buffer locking is special (hints, checksums, AIO writes)
Date: Wed, 25 Mar 2026 18:35:55 -0400
Message-ID: <yn4f45z6wl6wstuwha3qjsicrc6uvymkldtqz2ttchz7zdue3y@n6fll5dcter4> (raw)
In-Reply-To: <CAAKRu_Z=xrBWhAB5wRG8x-gnjNa7ZnW-sSsEbBfR1C8gwuTD8g@mail.gmail.com>
References: <4csodkvvfbfloxxjlkgsnl2lgfv2mtzdl7phqzd4jxjadxm4o5@usw7feyb5bzf>
<CALdSSPgyc7VuMLUZ8J7v2G-PebBa_vEi+mp-cLkcOwNycB56Hw@mail.gmail.com>
<kjdgvws34gpnz4y6xu5aaeul5mspgy3ahvyiw4lndb3ecsacdb@b2ccyowotpqh>
<jtg5cu4n6h5lib3kzx66ju4yhh6kmviaud7oq6dtut6c4q4rdi@xwsfoagt3c2b>
<k5j77f3q6ztihnjnx2nqxzyor6fbj2qxcbhzuxhkh2yy63jyfg@p72phigar3n4>
<5ubipyssiju5twkb7zgqwdr7q2vhpkpmuelxfpanetlk6ofnop@hvxb4g2amb2d>
<[email protected]>
<[email protected]>
<mheeefrtikvgjnjsenocvo3afj7vlpr5rljkzevssxs77n2zdt@sugqtjhryydo>
<CAAKRu_Z=xrBWhAB5wRG8x-gnjNa7ZnW-sSsEbBfR1C8gwuTD8g@mail.gmail.com>
Hi,
On 2026-03-25 17:34:33 -0400, Melanie Plageman wrote:
> On Wed, Mar 11, 2026 at 6:40 PM Andres Freund <[email protected]> wrote:
> >
> > I pushed this and many of the later patches in the series. Here are updated
> > versions of the remaining changes. The last two previously were one commit
> > with "WIP" in the title. The first one has, I think, not had a lot of review -
> > 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
Thanks for catching these.
Updated the PageSetChecksum() comment to
* Set checksum on a page.
*
* If the page is in shared buffers, it needs to be locked in at least
* share-exclusive mode.
...
> 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 = ReleaseAndReadBuffer(obuf, rel, blkno);
> - _bt_lockbuf(rel, buf, access);
> + {
> + if (BufferGetBlockNumber(obuf) == blkno)
> + {
> + /* trade in old lock mode for new lock */
> + _bt_unlockbuf(rel, obuf);
> + buf = obuf;
> + }
> + else
> + {
> + /* release lock and pin at once, that's a bit more efficient */
> + _bt_relbuf(rel, obuf);
> + buf = ReadBuffer(rel, blkno);
> + }
> + }
> + else
> + buf = 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?
I think it's very unlikely that it's called at any frequency with the same
buffer and lockmode. What would be the point of calling _bt_relandgetbuf() if
that's the case.
> 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?
Yea, it's a single index, so there can't be a different relfilenode.
> 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.
Good catch Melai.
> Also, I'd say this comment
> + /*
> + * Now okay to allow cancel/die interrupts again, were held when the lock
> + * was acquired.
> + */
>
> needs a "which" after the comma to read smoothly.
Fixed.
Running it through valgrind and then will work on reading through one more
time and pushing them.
Greetings,
Andres Freund
view thread (83+ 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: <yn4f45z6wl6wstuwha3qjsicrc6uvymkldtqz2ttchz7zdue3y@n6fll5dcter4>
* 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