Re: [PATCHES] scan_recycle_buffers
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 <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 7: You can help support the PostgreSQL project by donating at http://www.postgresql.org/about/donate
Re: [PATCHES] scan_recycle_buffers
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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] scan_recycle_buffers
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 +, Simon Riggs wrote: > > On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote: > > > "Simon Riggs" <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> http://momjian.us EnterpriseDB http://www.enterprisedb.com + If your life is a hard drive, Christ can be your backup. + ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] scan_recycle_buffers
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 s122 MB/s > 128KB79 s155 MB/s [1] > > Now apply cycle scan v2: > > Shared Buffers Scan_recycle_buffers Elapsed vmstat IO rate > -- --- - > 400MB 0101 s122 MB/s > 400MB 878 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] scan_recycle_buffers
Simon Riggs wrote: On Sat, 2007-03-10 at 07:59 +, Simon Riggs wrote: On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote: "Simon Riggs" <[EMAIL PROTECTED]> 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 s122 MB/s 128KB79 s155 MB/s [1] Now apply cycle scan v2: Shared Buffers Scan_recycle_buffers Elapsed vmstat IO rate -- --- - 400MB 0101 s122 MB/s 400MB 878 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. ---(end of broadcast)--- TIP 6: explain analyze is your friend
Re: [PATCHES] scan_recycle_buffers
On Sat, 2007-03-10 at 07:59 +, Simon Riggs wrote:
> On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote:
> > "Simon Riggs" <[EMAIL PROTECTED]> 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
Index: src/backend/commands/copy.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/commands/copy.c,v
retrieving revision 1.277
diff -c -r1.277 copy.c
*** src/backend/commands/copy.c 3 Mar 2007 19:32:54 - 1.277
--- src/backend/commands/copy.c 10 Mar 2007 08:45:50 -
***
*** 1865,1870
--- 1865,1872
&& !XLogArchivingActive())
use_wal = false;
+ StrategyHintRecycleBuffers(NScanRecycleBuffers);
+
/* Initialize state variables */
cstate->fe_eof = false;
cstate->eol_type = EOL_UNKNOWN;
***
*** 2115,2120
--- 2117,2124
}
}
+ StrategyHintRecycleBuffers(0);
+
/*
* If we skipped writing WAL for heaps, then we need to sync
*/
Index: src/backend/executor/nodeSeqscan.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/executor/nodeSeqscan.c,v
retrieving revision 1.63
diff -c -r1.63 nodeSeqscan.c
*** src/backend/executor/nodeSeqscan.c 5 Jan 2007 22:19:28 - 1.63
--- src/backend/executor/nodeSeqscan.c 10 Mar 2007 08:45:51 -
***
*** 24,29
--- 24,30
*/
#include "postgres.h"
+ #include "miscadmin.h"
#include "access/heapam.h"
#include "executor/execdebug.h"
#include "executor/nodeSeqscan.h"
***
*** 154,159
--- 155,166
estate->es_snapshot,
0,
NULL);
+ /*
+ * For scans larger than shared buffer cache enable
+ * buffer recycling, if the user has requested it
+ */
+ if (currentScanDesc->rs_nblocks > NBuffers && NScanRecycleBuffers > 0)
+ StrategyHintRecycleBuffers(NScanRecycleBuffers);
node->ss_currentRelation = currentRelation;
node->ss_currentScanDesc = currentScanDesc;
***
*** 272,277
--- 279,286
* close the heap relation.
*/
ExecCloseScanRelation(relation);
+
+ StrategyHintRecycleBuffers(0);
}
/*
Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.215
diff -c -r1.215 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 1 Feb 2007 19:10:27 - 1.215
--- src/backend/storage/buffer/bufmgr.c 10 Mar 2007 08:45:55 -
***
*** 320,325
--- 320,326
int buf_id;
volatile BufferDesc *buf;
bool valid;
+ bool lock_held = false;
/* create a tag so we can lookup the buffer */
INIT_BUFFERTAG(newTag, reln, blockNum);
***
*** 384,390
* it would be bad to hold the spinlock while possibly waking up other
* processes.
*/
! buf = StrategyGetBuffer();
Assert(buf->refcount == 0);
--- 385,391
* it would be bad to hold the spinlock while possibly waking up other
* processes.
*/
! buf = StrategyGetBuffer(&lock_held);
Assert(buf->refcount == 0);
***
*** 395,401
PinBuffer_Locked(buf);
/* Now it's safe to release the freelist lock */
! LWLockRelease(BufFreelistLock);
/*
* If the buffer was dirty, try to write it out. There is a race
--- 396,403
PinBuffer_Locked(buf);
/* Now it's safe to release the freelist lock */
! if (lock_held)
! LWLockRelease(BufFreelistLock);
/*
* If the buffer was dirty, try to write it out. There is a race
***
*** 884,891
PrivateRefCount[b]--;
if (PrivateRefCount[b] == 0)
{
- bool immed_free_buffer = false;
-
/* I'd better not still hold any locks on the buffer */
Assert(!LWLockHeldByMe(buf->content_lock));
Assert(!LWLockHeldByMe(buf->io_in_progress_lock));
--- 886,891
***
*** 899,915
/* Update buffer usage info, unless this is an internal access */
if (normalAccess)
{
if (!strategy_hint_vacuum)
{
if (buf->usage_count < BM_MAX_USAGE_COUNT)
buf->usage_count++;
}
- else
- {
- /* VACUUM accesses don't bump usage count, instead... */
- if (buf->refc
Re: [PATCHES] scan_recycle_buffers
On Fri, 2007-03-09 at 18:05 -0500, Tom Lane wrote: > "Simon Riggs" <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
Re: [PATCHES] scan_recycle_buffers
On Fri, 2007-03-09 at 20:08 +, 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] scan_recycle_buffers
On Fri, 2007-03-09 at 20:37 +, 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 ---(end of broadcast)--- TIP 2: Don't 'kill -9' the postmaster
Re: [PATCHES] scan_recycle_buffers
"Simon Riggs" <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] scan_recycle_buffers
On Fri, 2007-03-09 at 16:45 -0500, Tom Lane wrote: > Heikki Linnakangas <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 3: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] scan_recycle_buffers
Heikki Linnakangas <[EMAIL PROTECTED]> 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 ---(end of broadcast)--- TIP 9: In versions below 8.0, the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] scan_recycle_buffers
On Fri, 2007-03-09 at 20:08 +, 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 ---(end of broadcast)--- TIP 4: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] scan_recycle_buffers
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 ---(end of broadcast)--- TIP 5: don't forget to increase your free space map settings
