Re: Move unused buffers to freelist - Mailing list pgsql-hackers

On Wednesday, May 15, 2013 12:44 AM Greg Smith wrote:
> On 5/14/13 9:42 AM, Amit Kapila wrote:
> > In the attached patch, bgwriter/checkpointer moves unused
> (usage_count
> > =0 && refcount = 0) buffer's to end of freelist. I have implemented a
> > new API StrategyMoveBufferToFreeListEnd() to
> 
> There's a comment in the new function:
> 
> It is possible that we are told to put something in the freelist that
> is already in it; don't screw up the list if so.
> 
> I don't see where the code does anything to handle that though.  What
> was your intention here?

The intention is that put the entry in freelist only if it is not in
freelist which is accomplished by check
If (buf->freeNext == FREENEXT_NOT_IN_LIST). Every entry when removed from
freelist, buf->freeNext is marked as FREENEXT_NOT_IN_LIST.
Code Reference (last line):
StrategyGetBuffer()
{
..
..
while (StrategyControl->firstFreeBuffer >= 0)        {                buf =
&BufferDescriptors[StrategyControl->firstFreeBuffer];               Assert(buf->freeNext != FREENEXT_NOT_IN_LIST); 
 
               /* Unconditionally remove buffer from freelist */                StrategyControl->firstFreeBuffer =
buf->freeNext;               buf->freeNext = FREENEXT_NOT_IN_LIST;
 

...
}

Also the same check exists in StrategyFreeBuffer().

> This area has always been the tricky part of the change.  If you do
> something complicated when adding new entries, like scanning the
> freelist for duplicates, you run the risk of holding BufFreelistLock
> for
> too long. 

Yes, this is true and I had tried to hold this lock for minimal time. 
In this patch, it holds BufFreelistLock only to put the unused buffer at end
of freelist.

> To try and see that in benchmarks, I would use a small
> database scale (I typically use 100 for this type of test) and a large
> number of clients.  

>"-M prepared" would help get a higher transaction
> rate out of the hardware too.  It might take a server with a large core
> count to notice any issues with holding the lock for too long though.

This is good idea, I shall take another set of readings with "-M prepared"
> Instead you might just invalidate buffers before they go onto the list.
>   Doing that will then throw away usefully cached data though.

Yes, if we invalidate buffers, it might throw away usefully cached data
especially when working set just a tiny bit smaller than shared_buffers.
This is pointed by Robert in his mail
http://www.postgresql.org/message-id/CA+TgmoYhWsz__KtSxm6BuBirE7VR6Qqc_COkbE
ZTQpk8oom3CA@mail.gmail.com


> To try and optimize both insertion speed and retaining cached data,

I think by the method proposed by patch it takes care of both, because it
directly puts free buffer at end of freelist and 
because it doesn't invalidate the buffers it can retain cached data for
longer period.
Do you see any flaw with current approach?

> I
> was thinking about using a hash table for the free buffers, instead of
> the simple linked list approach used in the code now.

Okay, we can try different methods for maintaining free buffers if we find
current approach doesn't turn out to be good.
> Also:  check the formatting on the additions to in bufmgr.c, I noticed
> a
> spaces vs. tabs difference on lines 35/36 of your patch.

Thanks for pointing it, I shall send an updated patch along with next set of
performance data.


With Regards,
Amit Kapila.