On Fri, Jan 31, 2025 at 12:16 AM Ashutosh Bapat <ashutosh.bapat....@gmail.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
