Hi,
I went through this part of BgBufferSync() as well, and I agree the
patch is a nice improvement. Hoisting the reusable_buffers++
out of the two branches makes the real condition (BUF_REUSABLE)
explicit, and the Assert captures a real invariant of this call path:
since we pass skip_recently_used = true, SyncOneBuffer() can only
return BUF_WRITTEN together with BUF_REUSABLE, never alone.
Just a small wording nit on the comment. As written,
/*
* We instructed SyncOneBuffer not to write a recently used
* buffer.
*/
Assert(sync_state & BUF_REUSABLE);
it states the precondition rather than the invariant the Assert is
actually checking. I'd find it easier to read if it said the consequence
directly, e.g.
/*
* skip_recently_used is true, so SyncOneBuffer() only writes a
* buffer when it's reusable (refcount 0 and usage count 0).
*/
Assert(sync_state & BUF_REUSABLE);
Purely cosmetic, of course, the patch is correct either way.
With or without that tweak, it looks good to me.
Regards,
Aidar Imamov
> On Aug 17, 2026, at 19:58, Ashutosh Bapat <[email protected]>
> wrote:
>
> On Mon, Aug 17, 2026 at 9:42 PM Ashutosh Bapat
> <[email protected]> wrote:
>>
>> On Mon, May 5, 2025 at 7:07 PM Xuneng Zhou <[email protected]> wrote:
>>>
>>> Here's a rebase.
>>
>> Sorry for a very delayed response. Adding this to the next commitfest
>> so as not to forget it again.
>
> And rebased as required by CFBot. No actual conflict.
>
> --
> Best Wishes,
> Ashutosh Bapat
> <v20260817-0001-BgBufferSync-refactor-reusable_buffers-inc.patch>
