BgBufferSync(): clarification about reusable_buffers variable

Hi Marcel,

On Sat, Feb 8, 2025 at 6:24 AM M vd H <mvdholst@gmail.com> wrote:

I have been reading the source code of the BgWriter, and there is some
code in BgBufferSync() that I don't fully understand.

In BgBufferSync(), we have the following code:

while (num_to_scan > 0 && reusable_buffers < upcoming_alloc_est)
{
int sync_state = SyncOneBuffer(next_to_clean, true,
wb_context);

if (++next_to_clean >= NBuffers)
{
next_to_clean = 0;
next_passes++;
}
num_to_scan--;

if (sync_state & BUF_WRITTEN)
{
reusable_buffers++;
if (++num_written >= bgwriter_lru_maxpages)
{
PendingBgWriterStats.maxwritten_clean++;
break;
}
}
else if (sync_state & BUF_REUSABLE)
reusable_buffers++;
}

In SyncOneBuffer(), we lock the bufHdr and then check if both the
refcount and usage_count are zero. If so, we mark the return value as
BUF_REUSABLE.
My understanding is that this means that the buffer could be reused
when am empty shared buffer is needed by a backend. However, in the
code above, we seem to track these in the reusable_buffers variable.
But that variable is always incremented when the buffer was written in
SyncOneBuffer() even though that buffer might have a non-zero refcount
or non-zero usage_count.

I also think tha reusable_buffers keep track of the number of reusable
buffers. BgBufferSync() calls SyncOneBuffer() with skip_recently_used
= true. In that case, if SyncOneBuffer() finds the buffer with
refcount or usage_count non-zero, it just unlocks the header and
returns. Hence when called from BgBufferSync(), SyncOneBuffer() would
write a buffer only when it is not used. Hence the result would be 0
or BUF_REUSABLE or BUF_REUSABLE | BUF_WRITTEN. It can never be just
BUF_WRITTEN.

I guess, a patch like the one attached will be more readable and clear.

I ran pgbench for 5 minutes with this patch applied and didn't see the
Assert failing. But I don't think that's a good enough test to cover
all scenarios.

--
Best Wishes,
Ashutosh Bapat

Hi, tks for working on this. I had a chance to look at this while
googling BgBufferSync
function.

I also think tha reusable_buffers keep track of the number of reusable
buffers. BgBufferSync() calls SyncOneBuffer() with skip_recently_used
= true. In that case, if SyncOneBuffer() finds the buffer with
refcount or usage_count non-zero, it just unlocks the header and
returns. Hence when called from BgBufferSync(), SyncOneBuffer() would
write a buffer only when it is not used. Hence the result would be 0
or BUF_REUSABLE or BUF_REUSABLE | BUF_WRITTEN. It can never be just
BUF_WRITTEN.

Agrees. For this call stack, if skip_recently_used is set to be true,
sync_state cannot be BUF_WRITTEN alone.

I guess, a patch like the one attached will be more readable and clear.

I'm new to this part of code, and I found the patch version seems to be
more straightforward and less prone to misinterpretation.

I ran pgbench for 5 minutes with this patch applied and didn't see the
Assert failing. But I don't think that's a good enough test to cover
all scenarios.

The patch LGTM.

--

Here's a rebase.

On Mon, May 5, 2025 at 7:07 PM Xuneng Zhou <xunengzhou@gmail.com> wrote:

Here's a rebase.

Sorry for a very delayed response. Adding this to the next commitfest
so as not to forget it again.

--
Best Wishes,
Ashutosh Bapat

On Mon, Aug 17, 2026 at 9:42 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:

On Mon, May 5, 2025 at 7:07 PM Xuneng Zhou <xunengzhou@gmail.com> 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

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