Re: [PATCHES] [HACKERS] Bgwriter behavior
Later version of this patch added to the patch queue. Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches It will be applied as soon as one of the PostgreSQL committers reviews and approves it. --- Simon Riggs wrote: On Sat, 2005-01-01 at 17:47, Simon Riggs wrote: On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote: Simon Riggs wrote: Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I have no problem doing something for 8.0 if we can find something that meets all the items I mentioned. One idea would be to just remove bgwriter_percent. Beta/RC users would still have it in their postgresql.conf, but it is commented out so it should be OK. If they uncomment it their server would not start but we could just tell testers to remove it. I see that as better than having conflicting parameters. Can't say I like that at first thought. I'll think some more though... Another idea is to have bgwriter_percent be the percent of the buffer it will scan. Hmmmwell that was my original suggestion (bg2.patch on 12 Dec) (...though with a bug, as Neil pointed out) We could default that to 50% or 100%, but we then need to make sure all beta/RC users update their postgresql.conf with the new default because the commented-out default will not be correct. ...we just differ/ed on what the default should be... At this point I see these as our only two viable options, aside from doing nothing. I realize our current behavior requires a full scan of the buffer cache, but how often is the bgwriter_maxpages limit met? If it is not a full scan is done anyway, right? Well, if you heavy a very heavy read workload then that would be a problem. I was more worried about concurrency in a heavy write situation, but I can see your point, and agree. (Idea #1 still suffers from this, so we should rule it out...) It seems the only way to really add functionality is to change bgwriter_precent to control how much of the buffer is scanned. OK. I think you've persuaded me on idea #2, if I understand you right: bgwriter_percent = 50 (default) bgwriter_maxpages = 100 (default) percent is the number of shared_buffers we scan, limited by maxpages. (I'll code it up in a couple of hours when the kids are in bed) Here's the basic patch - no changes to current default values or docs. Not sure if this is still interesting or not... -- Best Regards, Simon Riggs [ Attachment, skipping... ] ---(end of broadcast)--- TIP 8: explain analyze is your friend -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Fri, 7 Jan 2005, Bruce Momjian wrote: Do we want to add this additional log infor to CVS for 8.0? No, unless we're looking for an RC5? --- Simon Riggs wrote: On Mon, 2005-01-03 at 19:14 -0500, Bruce Momjian wrote: Simon Riggs wrote: Here's my bgwriter instrumentation patch, which gives info that could allow the bgwriter settings to be tuned. Uh, what does this do exactly? Add additional logging output? Produces output like this... DEBUG:ARC T1target= 45 B1len= 4954 T1len= 40 T2len= 4960 B2len= 46 DEBUG:ARC total = 98% B1hit= 0% T1hit= 0% T2hit= 98% B2hit= 0% DEBUG:ARC buffer dirty misses= 22% (wasted=0); cleaned= 4494 when you have debug_shared_buffers (= n) set and you have server messages DEBUG1 available. The last line of log output has been replaced by this version. -- Best Regards, Simon Riggs ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED]) Marc G. Fournier Hub.Org Networking Services (http://www.hub.org) Email: [EMAIL PROTECTED] Yahoo!: yscrappy ICQ: 7615664 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Bgwriter behavior
Marc G. Fournier [EMAIL PROTECTED] writes: On Fri, 7 Jan 2005, Bruce Momjian wrote: Do we want to add this additional log infor to CVS for 8.0? No, unless we're looking for an RC5? I vote no as well. While it's probably not a dangerous change, the need for it has not been demonstrated. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] Bgwriter behavior
Tom Lane wrote: Marc G. Fournier [EMAIL PROTECTED] writes: On Fri, 7 Jan 2005, Bruce Momjian wrote: Do we want to add this additional log infor to CVS for 8.0? No, unless we're looking for an RC5? I vote no as well. While it's probably not a dangerous change, the need for it has not been demonstrated. OK, Simon, would you email me a copy of the patch again privately so I can put it in the 8.1 queue. I seem to have lost the email. Thanks. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [HACKERS] Bgwriter behavior
Do we want to add this additional log infor to CVS for 8.0? --- Simon Riggs wrote: On Mon, 2005-01-03 at 19:14 -0500, Bruce Momjian wrote: Simon Riggs wrote: Here's my bgwriter instrumentation patch, which gives info that could allow the bgwriter settings to be tuned. Uh, what does this do exactly? Add additional logging output? Produces output like this... DEBUG:ARC T1target= 45 B1len= 4954 T1len= 40 T2len= 4960 B2len= 46 DEBUG:ARC total = 98% B1hit= 0% T1hit= 0% T2hit= 98% B2hit= 0% DEBUG:ARC buffer dirty misses= 22% (wasted=0); cleaned= 4494 when you have debug_shared_buffers (= n) set and you have server messages DEBUG1 available. The last line of log output has been replaced by this version. -- Best Regards, Simon Riggs ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Mon, 2005-01-03 at 19:14 -0500, Bruce Momjian wrote: Simon Riggs wrote: Here's my bgwriter instrumentation patch, which gives info that could allow the bgwriter settings to be tuned. Uh, what does this do exactly? Add additional logging output? Produces output like this... DEBUG:ARC T1target= 45 B1len= 4954 T1len= 40 T2len= 4960 B2len= 46 DEBUG:ARC total = 98% B1hit= 0% T1hit= 0% T2hit= 98% B2hit= 0% DEBUG:ARC buffer dirty misses= 22% (wasted=0); cleaned= 4494 when you have debug_shared_buffers (= n) set and you have server messages DEBUG1 available. The last line of log output has been replaced by this version. -- Best Regards, Simon Riggs ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [HACKERS] Bgwriter behavior
OK, we have a submitted patch that attempts to improve bgwriter by making bgwriter_percent control what percentage of the buffer is scanned. The patch still needs doc changes and a change to the default value but at this point we need a vote on the patch. Is it: * too late for 8.0 * not the right improvement * to be applied with doc/default additions Comments? --- Simon Riggs wrote: On Sat, 2005-01-01 at 17:47, Simon Riggs wrote: On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote: Simon Riggs wrote: Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I have no problem doing something for 8.0 if we can find something that meets all the items I mentioned. One idea would be to just remove bgwriter_percent. Beta/RC users would still have it in their postgresql.conf, but it is commented out so it should be OK. If they uncomment it their server would not start but we could just tell testers to remove it. I see that as better than having conflicting parameters. Can't say I like that at first thought. I'll think some more though... Another idea is to have bgwriter_percent be the percent of the buffer it will scan. Hmmmwell that was my original suggestion (bg2.patch on 12 Dec) (...though with a bug, as Neil pointed out) We could default that to 50% or 100%, but we then need to make sure all beta/RC users update their postgresql.conf with the new default because the commented-out default will not be correct. ...we just differ/ed on what the default should be... At this point I see these as our only two viable options, aside from doing nothing. I realize our current behavior requires a full scan of the buffer cache, but how often is the bgwriter_maxpages limit met? If it is not a full scan is done anyway, right? Well, if you heavy a very heavy read workload then that would be a problem. I was more worried about concurrency in a heavy write situation, but I can see your point, and agree. (Idea #1 still suffers from this, so we should rule it out...) It seems the only way to really add functionality is to change bgwriter_precent to control how much of the buffer is scanned. OK. I think you've persuaded me on idea #2, if I understand you right: bgwriter_percent = 50 (default) bgwriter_maxpages = 100 (default) percent is the number of shared_buffers we scan, limited by maxpages. (I'll code it up in a couple of hours when the kids are in bed) Here's the basic patch - no changes to current default values or docs. Not sure if this is still interesting or not... -- Best Regards, Simon Riggs [ Attachment, skipping... ] ---(end of broadcast)--- TIP 8: explain analyze is your friend -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] [HACKERS] Bgwriter behavior
Bruce Momjian pgman@candle.pha.pa.us writes: OK, we have a submitted patch that attempts to improve bgwriter by making bgwriter_percent control what percentage of the buffer is scanned. The patch still needs doc changes and a change to the default value but at this point we need a vote on the patch. Is it: * too late for 8.0 * not the right improvement * to be applied with doc/default additions My vote: too late for 8.0. There is no hard evidence that this is a useful improvement, and no time for such evidence to be obtained. regards, tom lane ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Mon, 3 Jan 2005, Bruce Momjian wrote: OK, we have a submitted patch that attempts to improve bgwriter by making bgwriter_percent control what percentage of the buffer is scanned. The patch still needs doc changes and a change to the default value but at this point we need a vote on the patch. Is it: * too late for 8.0 Too late by at least 3 RCs ... Marc G. Fournier Hub.Org Networking Services (http://www.hub.org) Email: [EMAIL PROTECTED] Yahoo!: yscrappy ICQ: 7615664 ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Mon, 2005-01-03 at 20:09, Bruce Momjian wrote: OK, we have a submitted patch that attempts to improve bgwriter by making bgwriter_percent control what percentage of the buffer is scanned. The patch still needs doc changes and a change to the default value but at this point we need a vote on the patch. Is it: * too late for 8.0 * not the right improvement * to be applied with doc/default additions Comments? --- Simon Riggs wrote: On Sat, 2005-01-01 at 17:47, Simon Riggs wrote: On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote: Simon Riggs wrote: Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I hear veto ... so the above situation stands then: 8.1 it is. Not unhappy...I want this thing released as much as the next man... -- Best Regards, Simon Riggs ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] [HACKERS] Bgwriter behavior
Simon Riggs wrote: On Mon, 2005-01-03 at 20:09, Bruce Momjian wrote: OK, we have a submitted patch that attempts to improve bgwriter by making bgwriter_percent control what percentage of the buffer is scanned. The patch still needs doc changes and a change to the default value but at this point we need a vote on the patch. Is it: * too late for 8.0 * not the right improvement * to be applied with doc/default additions Comments? --- Simon Riggs wrote: On Sat, 2005-01-01 at 17:47, Simon Riggs wrote: On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote: Simon Riggs wrote: Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I hear veto ... so the above situation stands then: 8.1 it is. Not unhappy...I want this thing released as much as the next man... Well, we went through the process and that's the best we can do. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Mon, 2005-01-03 at 23:03, Bruce Momjian wrote:
Simon Riggs wrote:
On Mon, 2005-01-03 at 20:09, Bruce Momjian wrote:
OK, we have a submitted patch that attempts to improve bgwriter by
making bgwriter_percent control what percentage of the buffer is
scanned.
The patch still needs doc changes and a change to the default value but
at this point we need a vote on the patch. Is it:
* too late for 8.0
* not the right improvement
* to be applied with doc/default additions
Comments?
---
Simon Riggs wrote:
On Sat, 2005-01-01 at 17:47, Simon Riggs wrote:
On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote:
Simon Riggs wrote:
Well, I think we're saying: its not in 8.0 now, and we take our
time to
consider patches for 8.1 and accept the situation that the
parameter
names/meaning will change in next release.
I hear veto ... so the above situation stands then: 8.1 it is.
Not unhappy...I want this thing released as much as the next man...
Well, we went through the process and that's the best we can do.
Here's my bgwriter instrumentation patch, which gives info that could
allow the bgwriter settings to be tuned.
--
Best Regards, Simon Riggs
Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.182
diff -d -c -r1.182 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 24 Nov 2004 02:56:17 - 1.182
--- src/backend/storage/buffer/bufmgr.c 4 Jan 2005 00:04:18 -
***
*** 440,445
--- 440,446
UnpinBuffer(buf, true);
inProgress = FALSE;
buf = NULL;
+ StrategyBufferStatWastedIO();
}
}
} while (buf == NULL);
***
*** 682,687
--- 683,689
BufferDesc **dirty_buffers;
BufferTag *buftags;
int num_buffer_dirty;
+ int num_buffer_cleaned = 0;
int i;
/* If either limit is zero then we are disabled from doing anything... */
***
*** 770,775
--- 772,778
TerminateBufferIO(bufHdr, 0);
UnpinBuffer(bufHdr, true);
+ num_buffer_cleaned++;
}
LWLockRelease(BufMgrLock);
***
*** 777,782
--- 780,787
pfree(dirty_buffers);
pfree(buftags);
+ StrategyBufferStatCleaned(num_buffer_cleaned);
+
return num_buffer_dirty;
}
Index: src/backend/storage/buffer/freelist.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/freelist.c,v
retrieving revision 1.48
diff -d -c -r1.48 freelist.c
*** src/backend/storage/buffer/freelist.c 16 Sep 2004 16:58:31 - 1.48
--- src/backend/storage/buffer/freelist.c 4 Jan 2005 00:04:18 -
***
*** 115,120
--- 115,133
} while(0)
+ void
+ StrategyBufferStatWastedIO(void)
+ {
+ StrategyControl-num_wasted++;
+ }
+
+ void
+ StrategyBufferStatCleaned(long num_cleaned)
+ {
+ StrategyControl-num_cleaned += num_cleaned;
+ }
+
+
/*
* Printout for use when DebugSharedBuffers is enabled
*/
***
*** 130,159
t1_hit,
t2_hit,
b2_hit;
- int id,
- t1_clean,
- t2_clean;
ErrorContextCallback *errcxtold;
- id = StrategyControl-listHead[STRAT_LIST_T1];
- t1_clean = 0;
- while (id = 0)
- {
- if (BufferDescriptors[StrategyCDB[id].buf_id].flags BM_DIRTY)
- break;
- t1_clean++;
- id = StrategyCDB[id].next;
- }
- id = StrategyControl-listHead[STRAT_LIST_T2];
- t2_clean = 0;
- while (id = 0)
- {
- if (BufferDescriptors[StrategyCDB[id].buf_id].flags BM_DIRTY)
- break;
- t2_clean++;
- id = StrategyCDB[id].next;
- }
-
if (StrategyControl-num_lookup == 0)
all_hit = b1_hit = t1_hit = t2_hit = b2_hit = 0;
else
--- 143,150
***
*** 166,185
StrategyControl-num_lookup);
b2_hit = (StrategyControl-num_hit[STRAT_LIST_B2] * 100 /
StrategyControl-num_lookup);
! all_hit = b1_hit + t1_hit + t2_hit + b2_hit;
}
errcxtold = error_context_stack;
error_context_stack = NULL;
elog(DEBUG1, ARC T1target=%5d B1len=%5d T1len=%5d T2len=%5d B2len=%5d,
T1_TARGET, B1_LENGTH, T1_LENGTH, T2_LENGTH, B2_LENGTH);
! elog(DEBUG1, ARC total =%4ld%% B1hit=%4ld%% T1hit=%4ld%% T2hit=%4ld%% B2hit=%4ld%%,
all_hit, b1_hit, t1_hit, t2_hit, b2_hit);
! elog(DEBUG1, ARC clean buffers at LRU T1= %5d T2= %5d,
! t1_clean, t2_clean);
! error_context_stack = errcxtold;
StrategyControl-num_lookup = 0;
StrategyControl-num_hit[STRAT_LIST_B1] = 0;
StrategyControl-num_hit[STRAT_LIST_T1] = 0;
StrategyControl-num_hit[STRAT_LIST_T2] = 0;
--- 157,188
StrategyControl-num_lookup);
Re: [PATCHES] [HACKERS] Bgwriter behavior
Simon Riggs wrote: Here's my bgwriter instrumentation patch, which gives info that could allow the bgwriter settings to be tuned. Uh, what does this do exactly? Add additional logging output? -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] [HACKERS] Bgwriter behavior
This has been saved for the 8.1 release: http:/momjian.postgresql.org/cgi-bin/pgpatches2 --- Simon Riggs wrote: On Sat, 2005-01-01 at 17:47, Simon Riggs wrote: On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote: Simon Riggs wrote: Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I have no problem doing something for 8.0 if we can find something that meets all the items I mentioned. One idea would be to just remove bgwriter_percent. Beta/RC users would still have it in their postgresql.conf, but it is commented out so it should be OK. If they uncomment it their server would not start but we could just tell testers to remove it. I see that as better than having conflicting parameters. Can't say I like that at first thought. I'll think some more though... Another idea is to have bgwriter_percent be the percent of the buffer it will scan. Hmmmwell that was my original suggestion (bg2.patch on 12 Dec) (...though with a bug, as Neil pointed out) We could default that to 50% or 100%, but we then need to make sure all beta/RC users update their postgresql.conf with the new default because the commented-out default will not be correct. ...we just differ/ed on what the default should be... At this point I see these as our only two viable options, aside from doing nothing. I realize our current behavior requires a full scan of the buffer cache, but how often is the bgwriter_maxpages limit met? If it is not a full scan is done anyway, right? Well, if you heavy a very heavy read workload then that would be a problem. I was more worried about concurrency in a heavy write situation, but I can see your point, and agree. (Idea #1 still suffers from this, so we should rule it out...) It seems the only way to really add functionality is to change bgwriter_precent to control how much of the buffer is scanned. OK. I think you've persuaded me on idea #2, if I understand you right: bgwriter_percent = 50 (default) bgwriter_maxpages = 100 (default) percent is the number of shared_buffers we scan, limited by maxpages. (I'll code it up in a couple of hours when the kids are in bed) Here's the basic patch - no changes to current default values or docs. Not sure if this is still interesting or not... -- Best Regards, Simon Riggs [ Attachment, skipping... ] ---(end of broadcast)--- TIP 8: explain analyze is your friend -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faqs/FAQ.html
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Sat, 2005-01-01 at 06:20, Bruce Momjian wrote: This change isn't going to make it for RC3, and it probably not something we want to rush. OK. Thank you. I think there are a few issues involved: o everyone agrees the current meaning of bgwriter_percent is useless (percent of dirty buffers) o removal of bgwriter_percent will cause problems because postgresql.conf is only installed via initdb, so beta users will have to have some workaround so their existing postgresql.conf files work. o bgwriter_percent and bgwriter_maxpages are duplicate for a given number of buffers and it isn't clear which one takes precedence. o 8.1 might use these variables with different meanings, causing slight upgrade confusion. o Another idea is for bgwriter_percent to control how much of the buffer is scanned. Agreed. Would add as item #1: current behaviour of bgwriter causes sub-optimal performance for 8.0, for systems with a high write workload, more CPUs and higher shared_buffers. Tom feels bgwriter_maxpages is good because it allows the user to specify the I/O traffic, while bgwriter_percent as total pages (not just dirty ones) is perhaps easier to set a default (I/O load varies based on buffer cache size) and perhaps easier to understand. Agreed. I am not sure what to suggest at this point but whatever solution we use should take the above issues into account. Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. The patch is there if that decision changes, but I'll say no more on it. --- Simon Riggs wrote: On Fri, 2004-12-31 at 01:14, Bruce Momjian wrote: Simon Riggs wrote: On Mon, 2004-12-27 at 22:21, Bruce Momjian wrote: Should we consider at least adjusting the meaning of bgwriter_percent? Yes. As things stand, this is the only change that seems safe. Here's a very short patch that implements this change within BufferSync in bufmgr.c - No algorithm changes - No error message changes - Only change is the call to StrategyDirtyBufferList is made using the maximum number of buffers that will be cleaned, rather than uselessly trawling through all of shared_buffers This changes the meaning of bgwriter_percent from percent of dirty buffers to percent of shared_buffers. The default settings of 1% of 1000 buffers gives up to 10 dirty block writes every 250ms Benefit: allows performance tuning by increases options for setting bgwriter_delay which would otherwise have an ineffectually high minimum setting Risk: low 1-line doc patch to follow, if this is approved. I am not objecting to the patch, but what value is there in having both bgwriter_percent and bgwriter_maxpages? Seems both are redundant and that one would be enough. In brief: i) for now: as little change as possible is good ii) the two parameters are OK iii) trying to decide an alternative takes time, which we do not have iv) what is presented here is simply a performance bug fix, not the best long term alternative... I'd like to move quickly: if we do this (or an alternative), it has to be done soon and it would be easy to discuss this until we run out of time. Could we vote: in RC3, or not? In more detail... The value of having both is: i) as little change as possible at this stage of RC - the main one ...which gives us stability ...and also avoids having to re-discuss what they *should* be ii) Having two isn't that bad. bgwriter_percent auto adjusts the length of the to-be-cleaned-list, so it is roughly useful anywhere between 500 and 1 shared_buffers. That is IMHO slightly more useful than a hard definition set via bgwriter_maxpages, since that is likely to be set wrong anyway - but has some value as an outside limit on the number of pages. [You may wish to set shared_buffers 1 even on smaller servers, since many now have 2GB RAM and yet a relatively poor I/O subsystem. Having maxpages set separately allows the majority of people to set shared_buffers higher without swamping their I/O subsystems because they didn't know about the r8.0 bgwriter feature/parameters] iii) changing the parameters might tempt us towards changing the algorithm, which is not a topic we have reached agreement on iv) I see it as a goal to remove all of those parameters anyway, as well as explore some of the many options and ideas everybody has presented, so further change is likely at the next release whatever is done now. The patch is as simple as I can make it and yet remove the unnecessary performance effect in the existing
Re: [PATCHES] [HACKERS] Bgwriter behavior
Simon Riggs wrote: On Sat, 2005-01-01 at 06:20, Bruce Momjian wrote: This change isn't going to make it for RC3, and it probably not something we want to rush. OK. Thank you. I think there are a few issues involved: o everyone agrees the current meaning of bgwriter_percent is useless (percent of dirty buffers) o removal of bgwriter_percent will cause problems because postgresql.conf is only installed via initdb, so beta users will have to have some workaround so their existing postgresql.conf files work. o bgwriter_percent and bgwriter_maxpages are duplicate for a given number of buffers and it isn't clear which one takes precedence. o 8.1 might use these variables with different meanings, causing slight upgrade confusion. o Another idea is for bgwriter_percent to control how much of the buffer is scanned. Agreed. Would add as item #1: current behaviour of bgwriter causes sub-optimal performance for 8.0, for systems with a high write workload, more CPUs and higher shared_buffers. Tom feels bgwriter_maxpages is good because it allows the user to specify the I/O traffic, while bgwriter_percent as total pages (not just dirty ones) is perhaps easier to set a default (I/O load varies based on buffer cache size) and perhaps easier to understand. Agreed. I am not sure what to suggest at this point but whatever solution we use should take the above issues into account. Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I have no problem doing something for 8.0 if we can find something that meets all the items I mentioned. One idea would be to just remove bgwriter_percent. Beta/RC users would still have it in their postgresql.conf, but it is commented out so it should be OK. If they uncomment it their server would not start but we could just tell testers to remove it. I see that as better than having conflicting parameters. Another idea is to have bgwriter_percent be the percent of the buffer it will scan. We could default that to 50% or 100%, but we then need to make sure all beta/RC users update their postgresql.conf with the new default because the commented-out default will not be correct. At this point I see these as our only two viable options, aside from doing nothing. I realize our current behavior requires a full scan of the buffer cache, but how often is the bgwriter_maxpages limit met? If it is not a full scan is done anyway, right? It seems the only way to really add functionality is to change bgwriter_precent to control how much of the buffer is scanned. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote: Simon Riggs wrote: Well, I think we're saying: its not in 8.0 now, and we take our time to consider patches for 8.1 and accept the situation that the parameter names/meaning will change in next release. I have no problem doing something for 8.0 if we can find something that meets all the items I mentioned. One idea would be to just remove bgwriter_percent. Beta/RC users would still have it in their postgresql.conf, but it is commented out so it should be OK. If they uncomment it their server would not start but we could just tell testers to remove it. I see that as better than having conflicting parameters. Can't say I like that at first thought. I'll think some more though... Another idea is to have bgwriter_percent be the percent of the buffer it will scan. Hmmmwell that was my original suggestion (bg2.patch on 12 Dec) (...though with a bug, as Neil pointed out) We could default that to 50% or 100%, but we then need to make sure all beta/RC users update their postgresql.conf with the new default because the commented-out default will not be correct. ...we just differ/ed on what the default should be... At this point I see these as our only two viable options, aside from doing nothing. I realize our current behavior requires a full scan of the buffer cache, but how often is the bgwriter_maxpages limit met? If it is not a full scan is done anyway, right? Well, if you heavy a very heavy read workload then that would be a problem. I was more worried about concurrency in a heavy write situation, but I can see your point, and agree. (Idea #1 still suffers from this, so we should rule it out...) It seems the only way to really add functionality is to change bgwriter_precent to control how much of the buffer is scanned. OK. I think you've persuaded me on idea #2, if I understand you right: bgwriter_percent = 50 (default) bgwriter_maxpages = 100 (default) percent is the number of shared_buffers we scan, limited by maxpages. (I'll code it up in a couple of hours when the kids are in bed) -- Best Regards, Simon Riggs ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [HACKERS] Bgwriter behavior
Bruce Momjian pgman@candle.pha.pa.us writes: o everyone agrees the current meaning of bgwriter_percent is useless (percent of dirty buffers) Oh? It's not useless by any means; it's a perfectly reasonable and useful definition that happens to be expensive to implement. One of the questions that is not answered to my satisfaction is what is an adequate substitute that doesn't lose needed functionality. o bgwriter_percent and bgwriter_maxpages are duplicate for a given number of buffers and it isn't clear which one takes precedence. Not unless the current definition of bgwriter_percent is changed. Please try to make sure that your summaries reduce confusion instead of increasing it. regards, tom lane ---(end of broadcast)--- TIP 3: if posting/reading through Usenet, please send an appropriate subscribe-nomail command to [EMAIL PROTECTED] so that your message can get through to the mailing list cleanly
Re: [PATCHES] [HACKERS] Bgwriter behavior
On Sat, 2005-01-01 at 17:47, Simon Riggs wrote:
On Sat, 2005-01-01 at 17:01, Bruce Momjian wrote:
Simon Riggs wrote:
Well, I think we're saying: its not in 8.0 now, and we take our time to
consider patches for 8.1 and accept the situation that the parameter
names/meaning will change in next release.
I have no problem doing something for 8.0 if we can find something that
meets all the items I mentioned.
One idea would be to just remove bgwriter_percent. Beta/RC users would
still have it in their postgresql.conf, but it is commented out so it
should be OK. If they uncomment it their server would not start but we
could just tell testers to remove it. I see that as better than having
conflicting parameters.
Can't say I like that at first thought. I'll think some more though...
Another idea is to have bgwriter_percent be the percent of the buffer it
will scan.
Hmmmwell that was my original suggestion (bg2.patch on 12 Dec)
(...though with a bug, as Neil pointed out)
We could default that to 50% or 100%, but we then need to
make sure all beta/RC users update their postgresql.conf with the new
default because the commented-out default will not be correct.
...we just differ/ed on what the default should be...
At this point I see these as our only two viable options, aside from
doing nothing.
I realize our current behavior requires a full scan of the buffer cache,
but how often is the bgwriter_maxpages limit met? If it is not a full
scan is done anyway, right?
Well, if you heavy a very heavy read workload then that would be a
problem. I was more worried about concurrency in a heavy write
situation, but I can see your point, and agree.
(Idea #1 still suffers from this, so we should rule it out...)
It seems the only way to really add
functionality is to change bgwriter_precent to control how much of the
buffer is scanned.
OK. I think you've persuaded me on idea #2, if I understand you right:
bgwriter_percent = 50 (default)
bgwriter_maxpages = 100 (default)
percent is the number of shared_buffers we scan, limited by maxpages.
(I'll code it up in a couple of hours when the kids are in bed)
Here's the basic patch - no changes to current default values or docs.
Not sure if this is still interesting or not...
--
Best Regards, Simon Riggs
Index: src/backend/storage/buffer/bufmgr.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/storage/buffer/bufmgr.c,v
retrieving revision 1.182
diff -d -c -r1.182 bufmgr.c
*** src/backend/storage/buffer/bufmgr.c 24 Nov 2004 02:56:17 - 1.182
--- src/backend/storage/buffer/bufmgr.c 1 Jan 2005 21:03:16 -
***
*** 682,717
BufferDesc **dirty_buffers;
BufferTag *buftags;
int num_buffer_dirty;
int i;
/* If either limit is zero then we are disabled from doing anything... */
if (percent == 0 || maxpages == 0)
return 0;
/*
! * Get a list of all currently dirty buffers and how many there are.
* We do not flush buffers that get dirtied after we started. They
! * have to wait until the next checkpoint.
*/
! dirty_buffers = (BufferDesc **) palloc(NBuffers * sizeof(BufferDesc *));
! buftags = (BufferTag *) palloc(NBuffers * sizeof(BufferTag));
LWLockAcquire(BufMgrLock, LW_EXCLUSIVE);
- num_buffer_dirty = StrategyDirtyBufferList(dirty_buffers, buftags,
- NBuffers);
! /*
! * If called by the background writer, we are usually asked to only
! * write out some portion of dirty buffers now, to prevent the IO
! * storm at checkpoint time.
! */
! if (percent 0)
! {
! Assert(percent = 100);
! num_buffer_dirty = (num_buffer_dirty * percent + 99) / 100;
! }
! if (maxpages 0 num_buffer_dirty maxpages)
! num_buffer_dirty = maxpages;
/* Make sure we can handle the pin inside the loop */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
--- 682,728
BufferDesc **dirty_buffers;
BufferTag *buftags;
int num_buffer_dirty;
+ int max_buffer_dirty = 1;
+ int max_buffer_scan = 1;
int i;
/* If either limit is zero then we are disabled from doing anything... */
if (percent == 0 || maxpages == 0)
return 0;
+ /* Set number of buffers we will scan from LRUs of buffer lists */
+ if (percent 0 ) {
+ Assert(percent = 100);
+ max_buffer_scan = (NBuffers * percent + 99) / 100;
+ }
+
+ /* at checkpoint time we scan the whole buffer list */
+ if (percent 0)
+ max_buffer_scan = NBuffers;
+
+ if (maxpages 0 || maxpages NBuffers)
+ max_buffer_dirty = NBuffers;
+ else
+ max_buffer_dirty = maxpages;
+
+ /* we cannot find more dirty buffers than we scan */
+ if (max_buffer_dirty max_buffer_scan)
+ max_buffer_dirty = max_buffer_scan;
+
/*
! * Get a list of dirty buffers to clean and how
Re: [PATCHES] [HACKERS] Bgwriter behavior
Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: o everyone agrees the current meaning of bgwriter_percent is useless (percent of dirty buffers) Oh? It's not useless by any means; it's a perfectly reasonable and useful definition that happens to be expensive to implement. One of the questions that is not answered to my satisfaction is what is an adequate substitute that doesn't lose needed functionality. I remembered this statement: I think there's a reasonable case to be made for redefining bgwriter_percent as the max percent of the total buffer list to scan (not the max percent of the list to return --- Jan correctly pointed out that the latter is useless). Then we could modify StrategyDirtyBufferList so that the percent and maxpages parameters are passed in, so it can stop as soon as either one is satisfied. This would be a fairly small/safe code change and I wouldn't have a problem doing it even at this late stage of the cycle. Referenced here: http://archives.postgresql.org/pgsql-hackers/2004-12/msg00703.php But I now see that Jan was objecting to the idea of the previouis patch where bgwriter_percent is a percent of all buffers to return, which we just discussed as redundant. o bgwriter_percent and bgwriter_maxpages are duplicate for a given number of buffers and it isn't clear which one takes precedence. Not unless the current definition of bgwriter_percent is changed. Please try to make sure that your summaries reduce confusion instead of increasing it. OK, whatever. My point is that many have critisized the current behavior of bgwriter_percent and I haven't heard anyone defend it, including Jan. What bothers me is that we have known bgwriter needs tuning for months and I am not sure we are any closer to improving it. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 8: explain analyze is your friend
Re: [PATCHES] [HACKERS] Bgwriter behavior
This change isn't going to make it for RC3, and it probably not something we want to rush. I think there are a few issues involved: o everyone agrees the current meaning of bgwriter_percent is useless (percent of dirty buffers) o removal of bgwriter_percent will cause problems because postgresql.conf is only installed via initdb, so beta users will have to have some workaround so their existing postgresql.conf files work. o bgwriter_percent and bgwriter_maxpages are duplicate for a given number of buffers and it isn't clear which one takes precedence. o 8.1 might use these variables with different meanings, causing slight upgrade confusion. o Another idea is for bgwriter_percent to control how much of the buffer is scanned. Tom feels bgwriter_maxpages is good because it allows the user to specify the I/O traffic, while bgwriter_percent as total pages (not just dirty ones) is perhaps easier to set a default (I/O load varies based on buffer cache size) and perhaps easier to understand. I am not sure what to suggest at this point but whatever solution we use should take the above issues into account. --- Simon Riggs wrote: On Fri, 2004-12-31 at 01:14, Bruce Momjian wrote: Simon Riggs wrote: On Mon, 2004-12-27 at 22:21, Bruce Momjian wrote: Should we consider at least adjusting the meaning of bgwriter_percent? Yes. As things stand, this is the only change that seems safe. Here's a very short patch that implements this change within BufferSync in bufmgr.c - No algorithm changes - No error message changes - Only change is the call to StrategyDirtyBufferList is made using the maximum number of buffers that will be cleaned, rather than uselessly trawling through all of shared_buffers This changes the meaning of bgwriter_percent from percent of dirty buffers to percent of shared_buffers. The default settings of 1% of 1000 buffers gives up to 10 dirty block writes every 250ms Benefit: allows performance tuning by increases options for setting bgwriter_delay which would otherwise have an ineffectually high minimum setting Risk: low 1-line doc patch to follow, if this is approved. I am not objecting to the patch, but what value is there in having both bgwriter_percent and bgwriter_maxpages? Seems both are redundant and that one would be enough. In brief: i) for now: as little change as possible is good ii) the two parameters are OK iii) trying to decide an alternative takes time, which we do not have iv) what is presented here is simply a performance bug fix, not the best long term alternative... I'd like to move quickly: if we do this (or an alternative), it has to be done soon and it would be easy to discuss this until we run out of time. Could we vote: in RC3, or not? In more detail... The value of having both is: i) as little change as possible at this stage of RC - the main one ...which gives us stability ...and also avoids having to re-discuss what they *should* be ii) Having two isn't that bad. bgwriter_percent auto adjusts the length of the to-be-cleaned-list, so it is roughly useful anywhere between 500 and 1 shared_buffers. That is IMHO slightly more useful than a hard definition set via bgwriter_maxpages, since that is likely to be set wrong anyway - but has some value as an outside limit on the number of pages. [You may wish to set shared_buffers 1 even on smaller servers, since many now have 2GB RAM and yet a relatively poor I/O subsystem. Having maxpages set separately allows the majority of people to set shared_buffers higher without swamping their I/O subsystems because they didn't know about the r8.0 bgwriter feature/parameters] iii) changing the parameters might tempt us towards changing the algorithm, which is not a topic we have reached agreement on iv) I see it as a goal to remove all of those parameters anyway, as well as explore some of the many options and ideas everybody has presented, so further change is likely at the next release whatever is done now. The patch is as simple as I can make it and yet remove the unnecessary performance effect in the existing code. Thanks to Neil and others for showing that this was possible...I see this patch as a team effort. I've already spoken against larger change and would do so again now: if we don't agree this change, then I would vote for no-change simply because this patch is minimal change. We *suspect* further change is beneficial but we have no evidence to support what that change should be, amongst the large range of possible solutions proposed. -- Best Regards, Simon Riggs ---(end of broadcast)--- TIP 2: you can get
