Re: EvictUnpinnedBuffer and buffer free list

On Fri, Jan 31, 2025 at 12:16 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi All,
> EvictUnpinnedBuffer() calls InvalidateVictimBuffer() followed by
> UnpinBuffer() before returning. None of those functions put the buffer
> back into the free list. Its freeNext remains set to
> FREENEXT_NOT_IN_LIST. I think then that buffer will never be used and
> lost forever. I know that that function is only meant for development
> or testing but even while testing something losing a buffer forever
> seems like a problem.

As Yura says in a response to this mail, it won't be lost forever.
Those buffers can be found in the clocksweep.

Very few operations put buffers on the freelist. In the past, there
has been concern over contention on the
StrategyControl->buffer_strategy_lock. I found a few discussions about
this -- mostly in relation to having bgwriter put buffers on the
freelist. [1] and some other mentions about putting buffers on the
freelist to improve buffer eviction performance [2] and [3]. These are
all ancient though.

I don't have an explicit issue with EvictUnpinnedBuffer() putting
buffers on the freelist -- it seems like that could be fine. But since
it is for testing/development, I don't see what benefits it will have
to users. It sounds like you saw issues when developing -- what kinds
of issues?

> The prologue of function InvalidateVictimBuffer() says "/* Helper
> routine for GetVictimBuffer() ". I believe that it's expected that the
> buffer will be allocated to some other page, and that's why it doesn't
> return the buffer to the free list. But in the case of
> EvictUnpinnedBuffer() we are not using that buffer for any page, so it
> must be returned to the free list. InvalidateBuffer() does that but
> its prologue mentions that it's supposed to be used when freeing
> buffers for relations and databases.
>
> I think there are two solutions
> 1. Use InvalidBuffer() instead of InvalidateVictimBuffer(). But I am
> not sure whether that's safe or what other safety measures we have to
> put in EvictUnpinnedBuffer()

I don't really think we can do this. InvalidateBuffer() waits forever
to be able to put the buffer on the freelist. That's because it is
only used when dropping a relation or database. So it can assume (as
it says in the comments above WaitIO()) that the only reason the
buffer will be pinned is if someone else is flushing out the page. It
will always retry -- since the relation is being dropped, no one else
could be trying to concurrently access it to read it. You can't make
this assumption in EvictUnpinnedBuffer().

> 2. Call StrategyFreeBuffer() after InvalidateVictimBuffer()

I don't know exactly what would be required to make this work, but it
seems reasonable to try. The only places StrategyFreeBuffer() is used
is 1) InvalidateBuffer() and 2) when doing relation extension. In the
first case, we know no one can know about the buffer because we waited
until all pins were released and the buffer is part of a relation that
is being dropped. In the second case, I think the buffers we add to
the freelist are also ones that no one can know about yet because the
extension hasn't completed. I'm fuzzy on the details here, so I would
defer to Andres.

Anyway, my gut feeling is that we have to do something to make calling
StrategyFreeBuffer() safe to do in EvictUnpinnedBuffer(), but I don't
know what it is.

- Melanie

[1] https://www.postgresql.org/message-id/flat/00cc01ce5240%242da362b0%2488ea2810%24%40kapila%40huawei.com#7f6178995e3597bca130e8b89ca6f7ab
[2] https://www.postgresql.org/message-id/flat/CA%2BTgmoaP1PtRSkU0%3Dioi4hRxqCBzrNP9JV1L0YdkBp42PESSzw%40mail.gmail.com#9a4518544965d3e24f86ab79c4270810
[3] https://www.postgresql.org/message-id/flat/20140911110143.GV24649%40awork2.anarazel.de#9fd99d03652934f0d897ee88e68754ba