> On Jan 13, 2026, at 08:33, Andres Freund <[email protected]> wrote: > > Hi, > > On 2026-01-12 12:45:03 -0500, Andres Freund wrote: >> I'm doing another pass through 0003 and will push that if I don't find >> anything significant. > > Done, after adjust two comments in minor ways. > > >> Also working on doing comment polishing of the later patches, found a few >> things, but not quite enough to be worth reposting yet. > > Here are the remaining commits, with a bit of polish: > > - fixed references to old names in some places (lwlocks, release_ok) > > - Aded an assert that we don't already hold a lock in BufferLockConditional() > > - typo and grammar fixes > > - updated the commit message of the LW_FLAG_RELEASE_OK, as "requested" by > Melanie. I hope this explains the situation better. > > - added a commit that renames ResOwnerReleaseBufferPin to > ResOwnerReleaseBuffer (et al), as it now also releases content locks if held > > I kept this separate as I'm not yet sure about the new name, partially due > to there also being a "buffer io" resowner. I tried "buffer ownership" for > the resowner that tracks pins and locks, but that was long and not clearly > better. > > Greetings, > > Andres Freund > <v10-0001-lwlock-Invert-meaning-of-LW_FLAG_RELEASE_OK.patch><v10-0002-bufmgr-Make-definitions-related-to-buffer-descri.patch><v10-0003-bufmgr-Change-BufferDesc.state-to-be-a-64-bit-at.patch><v10-0004-bufmgr-Implement-buffer-content-locks-independen.patch><v10-0005-Require-share-exclusive-lock-to-set-hint-bits-an.patch><v10-0006-WIP-Make-UnlockReleaseBuffer-more-efficient.patch><v10-0007-WIP-bufmgr-Don-t-copy-pages-while-writing-out.patch><v10-0008-WIP-bufmgr-Rename-ResOwnerReleaseBufferPin.patch>
Hi Andres, So far I’ve only reviewed 0001 and 0002. I’m not very familiar with this area, so the review has been a bit slow. Overall, 0001 looks good to me. It renames LW_FLAG_RELEASE_OK to LW_FLAG_WAKE_IN_PROGRESS and inverts the meaning, which makes sense. I only have a small nit on naming: the local variable “new_release_in_progress". I see that it’s inherited from the old name and was updated from “_ok" to “_in_progress", but now that the flag itself is renamed, would it make sense to rename the variable as well? Something like “wake_in_progress" or “new_wake_in_progress" might better reflect the new flag name. In 0002, a bunch of new macros are introduced. My initial impression wasn’t great, mostly due to the amount of line wrapping. Looking a bit closer, I also noticed some duplication, for example, "BUF_REFCOUNT_BITS + BUF_USAGECOUNT_BITS" appears more than once; and a small inconsistency between BUF_STATE_GET_REFCOUNT and BUF_STATE_GET_USAGECOUNT (even though the former doesn’t actually need a shift). I tried a small refactor of the macro definitions in the attached diff to see if things could be made a bit more regular. It introduces a helper macro MASK() and a BUF_REFCOUNT_SHIFT constant, and removes a bit of duplication. If you like it, feel free to take it; otherwise, please just ignore it. Note that, the diff is based on 0002. (I actually hesitated to attach a diff, because if you’ve already created a CF entry, the attached diff could break the CI tests. If that happens, sorry about that.) Best regards, -- Chao Li (Evan) HighGo Software Co., Ltd. https://www.highgo.com/
buf_internals_h.diff
Description: Binary data
