Re: [WIP PATCH] for Performance Improvement in Buffer Management - Mailing list pgsql-hackers

On Tue, Sep 4, 2012 at 6:25 AM, Amit kapila <amit.kapila@huawei.com> wrote:
> On Tuesday, September 04, 2012 12:42 AM Jeff Janes wrote:
> On Mon, Sep 3, 2012 at 7:15 AM, Amit kapila <amit.kapila@huawei.com> wrote:
>>> This patch is based on below Todo Item:
>>
>>> Consider adding buffers the background writer finds reusable to the free
>>> list
>>
>>
>>
>>> I have tried implementing it and taken the readings for Select when all the
>>> data is in either OS buffers
>>
>>> or Shared Buffers.
>>
>>
>>
>>> The Patch has simple implementation for  "bgwriter or checkpoint process
>>> moving the unused buffers (unpinned with "ZERO" usage_count buffers) into
>>> "freelist".
>
>> I don't think InvalidateBuffer can be safely used in this way.  It
>> says "We assume
>> that no other backend could possibly be interested in using the page",
>> which is not true here.
>
> As I understood and anlyzed based on above, that there is problem in attached patch such that in function
> InvalidateBuffer(), after UnlockBufHdr() and before PartitionLock if some backend uses that buffer and increase the
usagecount to 1, still
 
> InvalidateBuffer() will remove the buffer from hash table and put it in Freelist.
> I have modified the code to address above by checking refcount & usage_count  inside Partition Lock
> , LockBufHdr and only after that move it to freelist which is similar to InvalidateBuffer.
> In actual code we can optimize the current code by using extra parameter in InvalidateBuffer.
>
> Please let me know if I understood you correctly or you want to say something else by above comment?

Yes, I think that this is part of the risk I was hinting at.  I
haven't evaluated your fix to it.  But assuming it is now safe, I
still think it is a bad idea to invalidate a perfectly good buffer.
Now a process that wants that page will have to read it in again, even
though it is still sitting there.  This is particularly bad because
the background writer is coded to always circle the buffer pool every
2 minutes, whether that many clean buffers are needed or not.  I think
that that is a bad idea, but having it invalidate buffers as it goes
is even worse.

I think the code for the free-list linked list is written so that it
performs correctly for a valid buffer to be on the freelist, even
though that does not happen under current implementations.  If you
find that a buffer on the freelist has become pinned, used, or dirty
since it was added (which can only happen if it is still valid), you
just remove it and try again.

>
>> Also, do we want to actually invalidate the buffers?  If someone does
>> happen to want one after it is put on the freelist, making it read it
>> in again into a different buffer doesn't seem like a nice thing to do,
>> rather than just letting it reclaim it.
>
> But even if bgwriter/checkpoint don't do, Backend needing new buffer will do similar things (remove from hash table)
forthis buffer as this is nextvictim buffer.
 

Right, but only if it is the nextvictim, here we do it if it is
nextvictim+N, for some largish values of N.  (And due to the 2 minutes
rule, sometimes for very large values of N)

I'm not sure how to devise a test case to prove that this can be
important, though.

Robert wrote an accounting patch a while ago that tallied how often a
buffer was cleaned but then reclaimed for the same page before being
evicted.  But now I can't find it.  If you can find that thread, there
might be some benchmarks posted to it that would be useful.


Cheers,

Jeff