Re: [PATCHES] scan_recycle_buffers

Patch to implement buffer cache recycling for scans, as being discussed
on pgsql-hackers.

Applies cleanly to cvstip, passes make installcheck when used by default
for all SeqScans. Tested with scan_recycle_buffers = 1,4,8,16

Should be regarded as WIP. Presumably there are some failure conditions
that require the buffer to be reset; these have not yet been considered.

No docs.

SET scan_recyle_buffers = N
default = 0

8 <= N <= 64 would yield benefits according to earlier results

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> Patch to implement buffer cache recycling for scans, as being discussed
> on pgsql-hackers.

A few questions come to mind:

How does it behave with Jeff's synchronized seq scans patch?

I wonder if calling RelationGetNumberOfBlocks on every seq scan becomes
a performance issue for tiny tables with for example just 1 page. It
performs an lseek, which isn't free.

What happens if multiple backends choose overlapping sets of buffers to
recycle?

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Fri, 2007-03-09 at 20:08 +0000, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Patch to implement buffer cache recycling for scans, as being discussed
> > on pgsql-hackers.
>
> A few questions come to mind:

Good questions. I don't expect this will go through easily, so we need
to examine these thoughts thoroughly.

> How does it behave with Jeff's synchronized seq scans patch?

I've offered Jeff lots of support throughout that patch's development
and its a feature I'd like to see. The current synch scan patch relies
upon the cache spoiling effect to gain its benefit. I think that can be
tightened up, so that we can make both work.

Currently synch scans help DSS apps but not OLTP. This patch reduces the
negative effects of VACUUM on OLTP workloads, as well as helping DSS.

> I wonder if calling RelationGetNumberOfBlocks on every seq scan becomes
> a performance issue for tiny tables with for example just 1 page. It
> performs an lseek, which isn't free.

Jeff's patch does this also, for similar reasons.

> What happens if multiple backends choose overlapping sets of buffers to
> recycle?

They won't. If a buffer is pinned, it will fall out of the the list of
buffers being recycled and not be reused. So they will each tend towards
a unique list of buffers.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
> I wonder if calling RelationGetNumberOfBlocks on every seq scan becomes
> a performance issue for tiny tables with for example just 1 page. It
> performs an lseek, which isn't free.

We do that anyway; but certainly Simon's patch ought not be injecting
an additional one.

regards, tom lane

On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote:
> Heikki Linnakangas <heikki(at)enterprisedb(dot)com> writes:
> > I wonder if calling RelationGetNumberOfBlocks on every seq scan becomes
> > a performance issue for tiny tables with for example just 1 page. It
> > performs an lseek, which isn't free.
>
> We do that anyway; but certainly Simon's patch ought not be injecting
> an additional one.

It should be possible to pass that down from the planner to the
executor, in certain cases. Or at least pass down the possibility that
such a check might be worthwhile.

Another approach might be to make the call after the first ~10 I/Os on a
SeqScan, after which an lseek will be just noise. That way an
all-in-cache scan would never need it at all. Thats easy to arrange
because the hint is invoked from the exec nodes themselves.

We probably need to get some measurements for the main benefit of the
patch before we look further into those thoughts.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote:
>> We do that anyway; but certainly Simon's patch ought not be injecting
>> an additional one.

> It should be possible to pass that down from the planner to the
> executor, in certain cases.

Huh? See HeapScanDesc->rs_nblocks.

regards, tom lane

On Fri, 2007-03-09 at 20:37 +0000, Simon Riggs wrote:
> > I wonder if calling RelationGetNumberOfBlocks on every seq scan becomes
> > a performance issue for tiny tables with for example just 1 page. It
> > performs an lseek, which isn't free.
>
> Jeff's patch does this also, for similar reasons.
>

As Tom pointed out, the value is already in memory by the time it gets
to my code. My code just reads that value from memory.

Regards,
Jeff Davis

On Fri, 2007-03-09 at 20:08 +0000, Heikki Linnakangas wrote:
> Simon Riggs wrote:
> > Patch to implement buffer cache recycling for scans, as being discussed
> > on pgsql-hackers.
>
> A few questions come to mind:
>
> How does it behave with Jeff's synchronized seq scans patch?
>

I will test it and post my results. I would expect that the CPU usage
will increase, but it might not make a big difference in the overall
cache hit rate if you count OS buffer cache hits.

Regards,
Jeff Davis

On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote:
> >> We do that anyway; but certainly Simon's patch ought not be injecting
> >> an additional one.
>
> > It should be possible to pass that down from the planner to the
> > executor, in certain cases.
>
> Huh? See HeapScanDesc->rs_nblocks.

Many thanks.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Sat, 2007-03-10 at 07:59 +0000, Simon Riggs wrote:
> On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote:
> > "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > > On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote:
> > >> We do that anyway; but certainly Simon's patch ought not be injecting
> > >> an additional one.
> >
> > > It should be possible to pass that down from the planner to the
> > > executor, in certain cases.
> >
> > Huh? See HeapScanDesc->rs_nblocks.
>
> Many thanks.

New patch enclosed, implementation as you've requested.

Not ready to apply yet, but good for testing.

COPY command now also uses this hint, to allow test results and
discussion. Others could also, perhaps needing different values.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

"Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:

> COPY command now also uses this hint, to allow test results and
> discussion. Others could also, perhaps needing different values.

Hm. It occurs to me that different commands may want different size buffer
rings.

As I understand it the reason your buffer rings are more than just a single
target buffer are:

1) For sequential scans because it creates a window for synchronized
sequential scans.

2) For dirty buffers because the dirty evicting the dirty buffer will force an
XLogFlush and we want to give a chance for the WAL pointer to advance past
the buffer's LSN. Ie, to allow other transactions to do our fsync for us
since it won't cost them much extra if anything.

Can you log whenever your ring buffer finds a provides a dirty buffer whose
LSN requires syncing the WAL log? That will help you figure out how large a
ring buffer you need to guarantee property 2.

--
Gregory Stark
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Sat, 2007-03-10 at 07:59 +0000, Simon Riggs wrote:
>> On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote:
>>> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
>>>> On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote:
>>>>> We do that anyway; but certainly Simon's patch ought not be injecting
>>>>> an additional one.
>>>> It should be possible to pass that down from the planner to the
>>>> executor, in certain cases.
>>> Huh? See HeapScanDesc->rs_nblocks.
>> Many thanks.
>
> New patch enclosed, implementation as you've requested.
>
> Not ready to apply yet, but good for testing.
>

A quick test using the setup for "Buffer cache is not scan resistant"
thread:

Firstly vanilla 8.3 from 20070310:

Shared Buffers Elapsed vmstat IO rate
-------------- ------- --------------
400MB 101 s 122 MB/s
128KB 79 s 155 MB/s [1]

Now apply cycle scan v2:

Shared Buffers Scan_recycle_buffers Elapsed vmstat IO rate
-------------- -------------------- ------- -------------
400MB 0 101 s 122 MB/s
400MB 8 78 s 155 MB/s
400MB 16 77 s 155 MB/s
400MB 32 78 s 155 MB/s
400MB 64 82 s 148 MB/s
400MB 128 93 s 128 MB/s

Certainly seems to have the desired effect!

Cheers

Mark

[1] I'm not seeing 166 MB/s like previous 8.2.3 data, however 8.3 PGDATA
is located further toward the end of the disk array - which I suspect is
limiting the IO rate a little.

On Sat, 2007-03-10 at 09:42 +0000, Gregory Stark wrote:
> "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
>
> > COPY command now also uses this hint, to allow test results and
> > discussion. Others could also, perhaps needing different values.
>
> Hm. It occurs to me that different commands may want different size buffer
> rings.

Yes, thats noted in comments in the patch. scan_recycle_buffers was
designed to allow us to test which types of scan benefit from which
settings.

> As I understand it the reason your buffer rings are more than just a single
> target buffer are:
>
> 1) For sequential scans because it creates a window for synchronized
> sequential scans.
>
> 2) For dirty buffers because the dirty evicting the dirty buffer will force an
> XLogFlush and we want to give a chance for the WAL pointer to advance past
> the buffer's LSN. Ie, to allow other transactions to do our fsync for us
> since it won't cost them much extra if anything.
>
> Can you log whenever your ring buffer finds a provides a dirty buffer whose
> LSN requires syncing the WAL log? That will help you figure out how large a
> ring buffer you need to guarantee property 2.

Hmm, again your thoughts mirrored my own, but this time you're slightly
ahead of me. I was just looking into the possibility of adaptive scans,
to allow synch scans to force the scan_recycle_buffer size higher. I
think having the size of the buffer vary during a scan seems sensible
also, within min and max limits.

I'll post some further thoughts tomorrow.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

On Sat, 2007-03-10 at 23:26 +1300, Mark Kirkwood wrote:
> Simon Riggs wrote:
> > New patch enclosed, implementation as you've requested.
> >
> > Not ready to apply yet, but good for testing.
> >
>
> A quick test using the setup for "Buffer cache is not scan resistant"
> thread:
>
> Firstly vanilla 8.3 from 20070310:
>
> Shared Buffers Elapsed vmstat IO rate
> -------------- ------- --------------
> 400MB 101 s 122 MB/s
> 128KB 79 s 155 MB/s [1]
>
> Now apply cycle scan v2:
>
> Shared Buffers Scan_recycle_buffers Elapsed vmstat IO rate
> -------------- -------------------- ------- -------------
> 400MB 0 101 s 122 MB/s
> 400MB 8 78 s 155 MB/s
> 400MB 16 77 s 155 MB/s
> 400MB 32 78 s 155 MB/s
> 400MB 64 82 s 148 MB/s
> 400MB 128 93 s 128 MB/s
>
> Certainly seems to have the desired effect!
>
> Cheers
>
> Mark
>
> [1] I'm not seeing 166 MB/s like previous 8.2.3 data, however 8.3 PGDATA
> is located further toward the end of the disk array - which I suspect is
> limiting the IO rate a little.

That's good news, thanks very much for testing that.

Before we can claim success, we need a few more tests on VACUUM, COPY
and a null test case to show it doesn't effect typical workloads, except
to improve vacuuming. I'll see if we can arrange those at EDB on a
reasonable size system.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Where is the final version of this patch? What patches are stuck in the
patch moderator queue?

---------------------------------------------------------------------------

Simon Riggs wrote:
> On Sat, 2007-03-10 at 07:59 +0000, Simon Riggs wrote:
> > On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote:
> > > "Simon Riggs" <simon(at)2ndquadrant(dot)com> writes:
> > > > On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote:
> > > >> We do that anyway; but certainly Simon's patch ought not be injecting
> > > >> an additional one.
> > >
> > > > It should be possible to pass that down from the planner to the
> > > > executor, in certain cases.
> > >
> > > Huh? See HeapScanDesc->rs_nblocks.
> >
> > Many thanks.
>
> New patch enclosed, implementation as you've requested.
>
> Not ready to apply yet, but good for testing.
>
> COPY command now also uses this hint, to allow test results and
> discussion. Others could also, perhaps needing different values.
>
> --
> Simon Riggs
> EnterpriseDB http://www.enterprisedb.com
>

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 6: explain analyze is your friend

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

On Mon, 2007-04-02 at 19:10 -0400, Bruce Momjian wrote:

> Where is the final version of this patch? What patches are stuck in the
> patch moderator queue?

We already discussed the dependency that exists with this patch and you
accepted that.

--
Simon Riggs
EnterpriseDB http://www.enterprisedb.com

Simon Riggs wrote:
> On Mon, 2007-04-02 at 19:10 -0400, Bruce Momjian wrote:
>
> > Where is the final version of this patch? What patches are stuck in the
> > patch moderator queue?
>
> We already discussed the dependency that exists with this patch and you
> accepted that.

Oh, that was the patch. I forgot. I am getting confused over which
patches are finished by the authors, and which are on hold because of
merge issues or open community discussion issues.

Rather than ask if patches are "completed", I think "finished" is a
better word, meaning the author has finished working on it, and it now
up to the community on how to proceed.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +