Thread
-
auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-13T22:19:17Z
On Thu, Jan 6, 2011 at 11:37 PM, Greg Smith <greg@2ndquadrant.com> wrote: > If it defaulted to 3% of shared_buffers, min 64K & max 16MB for the auto > setting, it would for the most part become an autotuned parameter. That > would make it 0.75 to 1MB at the standard anemic Linux default kernel > parameters. Maybe more than some would like, but dropping shared_buffers > from 24MB to 23MB to keep this from being ridiculously undersized is > probably a win. That percentage would reach 16MB by the time shared_buffers > was increased to 533MB, which also seems about right to me. On a really bad > setup (brief pause to flip off Apple) with only 4MB to work with total, > you'd end up with wal_buffers between 64 and 128K, so very close to the > status quo. > > Code that up, and we could probably even remove the parameter as a tunable > altogether. Very few would see a downside relative to any sensible > configuration under the current situation, and many people would notice > better automagic performance with one less parameter to tweak. Given the > recent investigations about the serious downsides of tiny wal_buffers values > on new Linux kernels when using open_datasync, a touch more aggression about > this setting seems particularly appropriate to consider now. That's been > swapped out as the default, but it's still possible people will switch to > it. Would anyone like to argue vigorously for or against the above proposal? I'll start: I think this is a good idea. I don't have a strong opinion on whether the exact details of Greg proposes above are precisely optimal, but I think they're in the right ballpark. Furthermore, we already have other things that are tuned in somewhat similar ways (e.g. the size of the fsync request queue defaults to the number of shared buffers) so there's precedent for it. It's one less parameter that you have to set to make things just work. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Magnus Hagander <magnus@hagander.net> — 2011-01-13T22:29:06Z
On Thu, Jan 13, 2011 at 23:19, Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, Jan 6, 2011 at 11:37 PM, Greg Smith <greg@2ndquadrant.com> wrote: >> If it defaulted to 3% of shared_buffers, min 64K & max 16MB for the auto >> setting, it would for the most part become an autotuned parameter. That >> would make it 0.75 to 1MB at the standard anemic Linux default kernel >> parameters. Maybe more than some would like, but dropping shared_buffers >> from 24MB to 23MB to keep this from being ridiculously undersized is >> probably a win. That percentage would reach 16MB by the time shared_buffers >> was increased to 533MB, which also seems about right to me. On a really bad >> setup (brief pause to flip off Apple) with only 4MB to work with total, >> you'd end up with wal_buffers between 64 and 128K, so very close to the >> status quo. >> >> Code that up, and we could probably even remove the parameter as a tunable >> altogether. Very few would see a downside relative to any sensible >> configuration under the current situation, and many people would notice >> better automagic performance with one less parameter to tweak. Given the >> recent investigations about the serious downsides of tiny wal_buffers values >> on new Linux kernels when using open_datasync, a touch more aggression about >> this setting seems particularly appropriate to consider now. That's been >> swapped out as the default, but it's still possible people will switch to >> it. > > Would anyone like to argue vigorously for or against the above proposal? > > I'll start: I think this is a good idea. I don't have a strong > opinion on whether the exact details of Greg proposes above are > precisely optimal, but I think they're in the right ballpark. > Furthermore, we already have other things that are tuned in somewhat > similar ways (e.g. the size of the fsync request queue defaults to the > number of shared buffers) so there's precedent for it. It's one less > parameter that you have to set to make things just work. +1, I like the idea. Would it still be there to override if necessary? -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-13T22:31:04Z
On Thu, Jan 13, 2011 at 5:29 PM, Magnus Hagander <magnus@hagander.net> wrote: > +1, I like the idea. Would it still be there to override if necessary? Depends what people want to do. We could make the default "0kB", and define that to mean "auto-tune", or we could remove the parameter altogether. I think I was envisioning the latter, but if people are hesitant to do that we could do the former instead. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Josh Berkus <josh@agliodbs.com> — 2011-01-13T23:02:12Z
> Depends what people want to do. We could make the default "0kB", and > define that to mean "auto-tune", or we could remove the parameter > altogether. I think I was envisioning the latter, but if people are > hesitant to do that we could do the former instead. Unfortunately, we might still need a manual parameter for override because of the interaction between wal_buffers and synchronous_commit=off, since it sets the max size of the unflushed data buffer. Discuss? And the "auto" setting should be -1, not 0kB. We use -1 for "use default" for several other GUCs. Other than that, I think Greg's numbers are fine, and strongly support having one less thing to tune. -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-13T23:06:24Z
On Thu, Jan 13, 2011 at 6:02 PM, Josh Berkus <josh@agliodbs.com> wrote: > >> Depends what people want to do. We could make the default "0kB", and >> define that to mean "auto-tune", or we could remove the parameter >> altogether. I think I was envisioning the latter, but if people are >> hesitant to do that we could do the former instead. > > Unfortunately, we might still need a manual parameter for override > because of the interaction between wal_buffers and > synchronous_commit=off, since it sets the max size of the unflushed data > buffer. Discuss? Do we have any evidence there's actually a problem in that case, or that a larger value of wal_buffers solves it? I mean, the background writer is going to start a background flush as quickly as it can... > And the "auto" setting should be -1, not 0kB. We use -1 for "use > default" for several other GUCs. No can do. Gotta have things in the same units. > Other than that, I think Greg's numbers are fine, and strongly support > having one less thing to tune. OK. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Josh Berkus <josh@agliodbs.com> — 2011-01-13T23:20:00Z
Robert, >> Unfortunately, we might still need a manual parameter for override >> because of the interaction between wal_buffers and >> synchronous_commit=off, since it sets the max size of the unflushed data >> buffer. Discuss? > > Do we have any evidence there's actually a problem in that case, or > that a larger value of wal_buffers solves it? I mean, the background > writer is going to start a background flush as quickly as it can... I don't think anyone has done any testing. However, the setting is there and some users might be convinced that they need it. >> And the "auto" setting should be -1, not 0kB. We use -1 for "use >> default" for several other GUCs. > > No can do. Gotta have things in the same units. That's certainly not true with, for example, log_temp_files. -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -
Re: auto-sizing wal_buffers
Kevin Grittner <kevin.grittner@wicourts.gov> — 2011-01-13T23:24:31Z
Robert Haas <robertmhaas@gmail.com> wrote: > Would anyone like to argue vigorously for or against the above > proposal? Greg's numbers look reasonable to me, and there's nobody I'd trust more to come up with reasonable numbers for this. One less tunable is a good thing, especially since this designed to scale from someone slapping it on his laptop for a first quick try, all the way up to industrial strength production environments. I guess a manual override doesn't bother me too much, but I am a bit dubious of its value, and there is value in keeping the GUC count down.... -Kevin
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-14T00:25:09Z
Robert Haas <robertmhaas@gmail.com> writes: > On Thu, Jan 13, 2011 at 5:29 PM, Magnus Hagander <magnus@hagander.net> wrote: >> +1, I like the idea. Would it still be there to override if necessary? > Depends what people want to do. We could make the default "0kB", and > define that to mean "auto-tune", or we could remove the parameter > altogether. I think I was envisioning the latter, but if people are > hesitant to do that we could do the former instead. I think we need to keep the override capability until the autotune algorithm has proven itself in the field for a couple of years. I agree with Josh that a negative value should be used to select the autotune method. regards, tom lane
-
Re: auto-sizing wal_buffers
Greg Smith <greg@2ndquadrant.com> — 2011-01-15T06:51:50Z
Tom Lane wrote: > I think we need to keep the override capability until the autotune > algorithm has proven itself in the field for a couple of years. > > I agree with Josh that a negative value should be used to select the > autotune method. > Agreed on both fronts. Attached patch does the magic. Also available in branch "walbuffers" from git://github.com/greg2ndQuadrant/postgres.git By changing only shared_buffers I get the following quite reasonable automatic behavior: $ psql -c "SELECT name,unit,boot_val,setting,current_setting(name) FROM pg_settings WHERE name IN ('wal_buffers','shared_buffers')" name | unit | boot_val | setting | current_setting ----------------+------+----------+---------+----------------- shared_buffers | 8kB | 1024 | 3072 | 24MB wal_buffers | 8kB | -1 | 96 | 768kB shared_buffers | 8kB | 1024 | 4096 | 32MB wal_buffers | 8kB | -1 | 128 | 1MB shared_buffers | 8kB | 1024 | 16384 | 128MB wal_buffers | 8kB | -1 | 512 | 4MB shared_buffers | 8kB | 1024 | 131072 | 1GB wal_buffers | 8kB | -1 | 2048 | 16MB shared_buffers | 8kB | 1024 | 262144 | 2GB wal_buffers | 8kB | -1 | 2048 | 16MB If you've set it to the auto-tuning behavior, you don't see that setting of -1 in the SHOW output; you see the value it's actually been set to. The only way to know that was set automatically is to look at boot_val as I've shown here. I consider this what admins would prefer, as the easy way to expose the value that was used. I would understand if people considered it a little odd though. Since you can't change it without a postgresql.conf edit and a server start anyway, and it's tersely documented in the sample postgresql.conf what -1 does, I don't see this being a problem for anyone in the field. To try and clear up some of the confusion around how the earlier documentation suggests larger values of this aren't needed, I added the following updated description of how this has been observed to work for admins in practice: ! Since the data is written out to disk at every transaction commit, ! the setting many only need to be be large enough to hold the amount ! of WAL data generated by one typical transaction. Larger values, ! typically at least a few megabytes, can improve write performance ! on a busy server where many clients are committing at once. ! Extremely large settings are unlikely to provide additional benefit. And to make this easy as possible to apply if I got this right, here's some proposed commit text: Automatically set wal_buffers to be proportional to the size of shared_buffers. Make it 1/32 as large when the auto-tuned behavior, which is the default and set with a value of -1, is used. The previous default of 64kB is still enforced as a minimum value. The maximum automatic value is limited to 16MB. (Note that this not exactly what I put in my own commit message if you grab from my repo, that had a typo) -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books -
Re: auto-sizing wal_buffers
Greg Smith <greg@2ndquadrant.com> — 2011-01-15T07:01:36Z
Kevin Grittner wrote: > I guess a manual override doesn't bother me too much, but I am a bit dubious of its > value, and there is value in keeping the GUC count down... It's a risk-reward thing really. The reward for removing it is that a few lines of code and a small section of the documentation go away. It's not very big. The risk seems low, but it's not zero. Let's say this goes in, we get to 9.2 or later, and a survey suggests that no one has needed to ever set wal_buffers when deploying 9.1. At that point I think everyone would feel much better considering to nuke it altogether. I just looked at the code again when developing the patch, and there's really not enough benefit to removing it to worry about taking any risk right now. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books
-
Re: auto-sizing wal_buffers
Fujii Masao <masao.fujii@gmail.com> — 2011-01-15T08:02:30Z
On Sat, Jan 15, 2011 at 3:51 PM, Greg Smith <greg@2ndquadrant.com> wrote: > Agreed on both fronts. Attached patch does the magic. Also available in > branch "walbuffers" from git://github.com/greg2ndQuadrant/postgres.git +int XLOGbuffersMin = 8; XLOGbuffersMin is a fixed value. I think that defining it as a macro rather than a variable seems better. + if (XLOGbuffers > 2048) + XLOGbuffers = 2048; Using "XLOG_SEG_SIZE/XLOG_BLCKSZ" rather than 2048 seems better. +#wal_buffers = -1 # min 32kB, -1 sets based on shared_buffers Typo: s/32kB/64kB Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
-
Re: auto-sizing wal_buffers
Greg Smith <greg@2ndquadrant.com> — 2011-01-15T16:52:52Z
Fujii Masao wrote: > +int XLOGbuffersMin = 8; > > XLOGbuffersMin is a fixed value. I think that defining it as a macro > rather than a variable seems better. > > + if (XLOGbuffers > 2048) > + XLOGbuffers = 2048; > > Using "XLOG_SEG_SIZE/XLOG_BLCKSZ" rather than 2048 seems > better. > > +#wal_buffers = -1 # min 32kB, -1 sets based on shared_buffers > > Typo: s/32kB/64kB > Thanks, I've fixed all these issues and attached a new full patch, pushed to github, etc. Tests give same results back, and it's nice that it scale to reasonable behavior if someone changes their XLOG segment size. It should be possible to set the value back to the older minimum value of 32kB too. That's doesn't actually seem to work though; when I try it I get: $ psql -c "SELECT name,unit,boot_val,setting,current_setting(name) FROM pg_settings WHERE name IN ('wal_buffers','shared_buffers')" name | unit | boot_val | setting | current_setting ----------------+------+----------+---------+----------------- shared_buffers | 8kB | 1024 | 131072 | 1GB wal_buffers | 8kB | -1 | 8 | 64kB Where I was expecting that setting to be "4" instead for 32kB. So there's probably some minor bug left in where I inserted this into the initialization sequence. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books -
Re: auto-sizing wal_buffers
Josh Berkus <josh@agliodbs.com> — 2011-01-15T22:34:11Z
On 1/14/11 10:51 PM, Greg Smith wrote: > > ! Since the data is written out to disk at every transaction > commit, > ! the setting many only need to be be large enough to hold the > amount > ! of WAL data generated by one typical transaction. Larger values, > ! typically at least a few megabytes, can improve write performance > ! on a busy server where many clients are committing at once. > ! Extremely large settings are unlikely to provide additional > benefit. I think we can be more specific on that last sentence; is there even any *theoretical* benefit to settings above 16MB, the size of a WAL segment? Certainly there have been no test results to show any. If we don't know, keep it vague, but otherwise I suggest: "Settings larger than the size of a single WAL segment (16MB by default) are unlikely to produce any benefit." -- -- Josh Berkus PostgreSQL Experts Inc. http://www.pgexperts.com -
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-16T17:32:30Z
Josh Berkus <josh@agliodbs.com> writes: > I think we can be more specific on that last sentence; is there even any > *theoretical* benefit to settings above 16MB, the size of a WAL segment? IIRC there's a forced fsync at WAL segment switch, so no. regards, tom lane
-
Re: auto-sizing wal_buffers
Marti Raudsepp <marti@juffo.org> — 2011-01-16T22:46:06Z
On Sun, Jan 16, 2011 at 00:34, Josh Berkus <josh@agliodbs.com> wrote: > I think we can be more specific on that last sentence; is there even any > *theoretical* benefit to settings above 16MB, the size of a WAL segment? > Certainly there have been no test results to show any. I don't know if it's applicable to real workloads in any way, but it did make a measurable difference in one of my tests. Back when benchmarking different wal_sync_methods, I found that when doing massive INSERTs from generate_series, the INSERT time kept improving even after increasing wal_buffers from 16MB to 32, 64 and 128MB; especially with wal_sync_method=open_datasync. The total INSERT+COMMIT time remained constant, however. More details here: http://archives.postgresql.org/pgsql-performance/2010-11/msg00094.php Regards, Marti
-
Re: auto-sizing wal_buffers
Fujii Masao <masao.fujii@gmail.com> — 2011-01-17T01:07:16Z
On Sun, Jan 16, 2011 at 1:52 AM, Greg Smith <greg@2ndquadrant.com> wrote: > Fujii Masao wrote: >> >> +int XLOGbuffersMin = 8; >> >> XLOGbuffersMin is a fixed value. I think that defining it as a macro >> rather than a variable seems better. >> >> + if (XLOGbuffers > 2048) >> + XLOGbuffers = 2048; >> >> Using "XLOG_SEG_SIZE/XLOG_BLCKSZ" rather than 2048 seems >> better. >> >> +#wal_buffers = -1 # min 32kB, -1 sets based on >> shared_buffers >> >> Typo: s/32kB/64kB >> > > Thanks, I've fixed all these issues and attached a new full patch, pushed to > github, etc. Tests give same results back, and it's nice that it scale to > reasonable behavior if someone changes their XLOG segment size. Thanks for the update. +/* Minimum setting used for a lower bound on wal_buffers */ +#define XLOG_BUFFER_MIN 4 Why didn't you use XLOG_BUFFER_MIN instead of XLOGbuffersMin? XLOG_BUFFER_MIN is not used anywhere for now. + if (XLOGbuffers < (XLOGbuffersMin * 2)) + XLOGbuffers = XLOGbuffersMin * 2; + } Why is the minimum value 64kB only when wal_buffers is set to -1? This seems confusing for users. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
-
Re: auto-sizing wal_buffers
Fujii Masao <masao.fujii@gmail.com> — 2011-01-17T01:15:46Z
On Sun, Jan 16, 2011 at 7:34 AM, Josh Berkus <josh@agliodbs.com> wrote: > I think we can be more specific on that last sentence; is there even any > *theoretical* benefit to settings above 16MB, the size of a WAL segment? > Certainly there have been no test results to show any. If the workload generates 16MB or more WAL for wal_writer_delay, 16MB or more of wal_buffers would be effective. In that case, wal_buffers is likely to be filled up with unwritten WAL, then you have to write buffers while holding WALInsert lock. This is obviously not good. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
-
Re: auto-sizing wal_buffers
Jeff Janes <jeff.janes@gmail.com> — 2011-01-17T03:58:01Z
On Sat, Jan 15, 2011 at 2:34 PM, Josh Berkus <josh@agliodbs.com> wrote: > On 1/14/11 10:51 PM, Greg Smith wrote: >> >> ! Since the data is written out to disk at every transaction >> commit, >> ! the setting many only need to be be large enough to hold the >> amount >> ! of WAL data generated by one typical transaction. Larger values, >> ! typically at least a few megabytes, can improve write performance >> ! on a busy server where many clients are committing at once. >> ! Extremely large settings are unlikely to provide additional >> benefit. > > I think we can be more specific on that last sentence; is there even any > *theoretical* benefit to settings above 16MB, the size of a WAL segment? I would turn it around and ask if there is any theoretical reason it would not benefit? (And if so, can they be cured soon?) > Certainly there have been no test results to show any. Did the tests show steady improvement up to 16MB and then suddenly hit a wall? (And in which case, were they recompiled at a larger segment size and repeated?) Or did improvement just peter out because 16MB is really quite a bit and there was just no need for it to be larger independent of segment size? Cheers, Jeff
-
Re: auto-sizing wal_buffers
Jeff Janes <jeff.janes@gmail.com> — 2011-01-17T04:06:53Z
On Sun, Jan 16, 2011 at 9:32 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Josh Berkus <josh@agliodbs.com> writes: >> I think we can be more specific on that last sentence; is there even any >> *theoretical* benefit to settings above 16MB, the size of a WAL segment? > > IIRC there's a forced fsync at WAL segment switch, so no. However other backends can still do WAL inserts while that fsync takes place, as long as they can find available buffers to write into. So that should not be too limiting--a larger wal_buffers make it more likely they will find available buffers. However if the background writer does not keep up under bulk loading conditions, then the end of segment fsync will probably happen via AdvanceXLInsertBuffer, which will be sitting on the WALInsertLock. So that is obviously bad news. Cheers, Jeff
-
Re: auto-sizing wal_buffers
Greg Smith <greg@2ndquadrant.com> — 2011-01-18T11:50:52Z
Fujii Masao wrote: > +/* Minimum setting used for a lower bound on wal_buffers */ > +#define XLOG_BUFFER_MIN 4 > > Why didn't you use XLOG_BUFFER_MIN instead of XLOGbuffersMin? > XLOG_BUFFER_MIN is not used anywhere for now. > That's a typo; will fix. > + if (XLOGbuffers < (XLOGbuffersMin * 2)) > + XLOGbuffers = XLOGbuffersMin * 2; > + } > > Why is the minimum value 64kB only when wal_buffers is set to > -1? This seems confusing for users. > That's because the current default on older versions is 64kB. Since the automatic selection is going to be the new default, I hope, I don't want it to be possible it will pick a number smaller than the default of older versions. So the automatic lower limit is 64kB, while the actual manually set lower limit remains 32kB, as before. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books
-
Re: auto-sizing wal_buffers
Greg Smith <greg@2ndquadrant.com> — 2011-01-18T11:55:34Z
Josh Berkus wrote: > I think we can be more specific on that last sentence; is there even any > *theoretical* benefit to settings above 16MB, the size of a WAL segment? > Certainly there have been no test results to show any. > There was the set Marti just reminded about. The old wording suggested big enough to fix a single transaction was big enough, and that let to many people being confused and setting this parameter way too low. Since it's possible I'm wrong about 16MB being the upper limit, I didn't want the wording to specifically rule out people testing that size to see what happens. We believe there's never any advantage due to the forced wal segment switch, but having test results to the contrary floating around keeps me from being too aggressive in how the wording there goes. -- Greg Smith 2ndQuadrant US greg@2ndQuadrant.com Baltimore, MD PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us "PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books
-
Re: auto-sizing wal_buffers
Fujii Masao <masao.fujii@gmail.com> — 2011-01-19T02:51:10Z
On Tue, Jan 18, 2011 at 8:50 PM, Greg Smith <greg@2ndquadrant.com> wrote: >> Why is the minimum value 64kB only when wal_buffers is set to >> -1? This seems confusing for users. >> > > That's because the current default on older versions is 64kB. Since the > automatic selection is going to be the new default, I hope, I don't want it > to be possible it will pick a number smaller than the default of older > versions. So the automatic lower limit is 64kB, while the actual manually > set lower limit remains 32kB, as before. It would be helpful to explain that as the source code comment. Also in the document. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-22T05:33:09Z
On Sat, Jan 15, 2011 at 11:52 AM, Greg Smith <greg@2ndquadrant.com> wrote: > Where I was expecting that setting to be "4" instead for 32kB. So there's > probably some minor bug left in where I inserted this into the > initialization sequence. So I started taking a look at this patch tonight with an eye to committing it, but ended up having to whack it around fairly hard to fix the problem described above: the problem is, I believe, that the initialization code in question doesn't run in every backend. My first thought was "gee, it's a bad idea for us to be manipulating the value of the GUC variable directly, I should use an assign_hook". But of course that turns out to be a bad idea, because there's no guarantee that wal_buffers will be initialized after shared_buffers. So I put it back the way you had it, and jiggered things so that the copy of the value that's actually used for memory allocation gets stored in XLogCtl. That made it possible to define a show_hook that DTRT, but that wasn't entirely straightforward either: the show_hook has to deliver the finished string, not just a replacement integer for guc.c to format. So I exposed the relevant formatting logic from guc.c as a separate function (duplicating that code didn't seem smart), and now it all seems to work. See attached, which also includes some other rewriting of code and docs that I hope constitute improvements. Barring screams of agony^W^W^Whelpful suggestions for how to code this more neatly, I'll go ahead and commit this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Jaime Casanova <jaime@2ndquadrant.com> — 2011-01-22T06:30:32Z
On Sat, Jan 22, 2011 at 12:33 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Sat, Jan 15, 2011 at 11:52 AM, Greg Smith <greg@2ndquadrant.com> wrote: >> Where I was expecting that setting to be "4" instead for 32kB. So there's >> probably some minor bug left in where I inserted this into the >> initialization sequence. > > So I exposed the relevant formatting logic from guc.c as a separate function i have read this very breafly, so not much comment... just a few questions... why is this better than using XLOG_BUFFER_MIN? (the same for the 8 buffers assigned just above of it) + else if (XLOGbuffers < 4) + XLOGbuffers = 4; also this + Assert(XLOGbuffers > 0); maybe should be Assert(XLOGbuffers >= XLOG_BUFFER_MIN); while you move the code, why didn't you keep this comment? - /* - * Use int64 arithmetic to avoid overflows in units - * conversion. - */ -- Jaime Casanova www.2ndQuadrant.com Professional PostgreSQL: Soporte y capacitación de PostgreSQL -
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-22T14:54:53Z
On Sat, Jan 22, 2011 at 1:30 AM, Jaime Casanova <jaime@2ndquadrant.com> wrote: > why is this better than using XLOG_BUFFER_MIN? (the same for the 8 > buffers assigned just above of it) > > + else if (XLOGbuffers < 4) > + XLOGbuffers = 4; Oh, good point. Woops. > also this > + Assert(XLOGbuffers > 0); > maybe should be > Assert(XLOGbuffers >= XLOG_BUFFER_MIN); I think that's slightly less clear about the point of the assertion, which is to make sure we're at least allocating something. > while you move the code, why didn't you keep this comment? > - /* > - * Use int64 arithmetic to avoid overflows in units > - * conversion. > - */ Because I suck. Will fix. Thanks for the fast and detailed review. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T00:22:26Z
Robert Haas <robertmhaas@gmail.com> writes: > Barring screams of agony^W^W^Whelpful suggestions for how to code this > more neatly, I'll go ahead and commit this. The show_hook sucks, and is unnecessary, as is exposing the format function. Just set the GUC variable to the correct value. regards, tom lane
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T01:33:54Z
I wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> Barring screams of agony^W^W^Whelpful suggestions for how to code this >> more neatly, I'll go ahead and commit this. > The show_hook sucks, and is unnecessary, as is exposing the format > function. Just set the GUC variable to the correct value. Um: that was probably too brief. "Just set" means "use SetConfigOption". As penance, I've committed the corrected patch. regards, tom lane
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-23T01:45:02Z
On Sat, Jan 22, 2011 at 8:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Um: that was probably too brief. Yep. > "Just set" means "use SetConfigOption". OK. > As penance, I've committed the corrected patch. Which is obviously derived from my version rather than Greg's, without credit. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T01:50:20Z
Robert Haas <robertmhaas@gmail.com> writes: >> As penance, I've committed the corrected patch. > Which is obviously derived from my version rather than Greg's, without credit. Apologies --- I thought I'd ended up reverting basically all your changes, so I just credited him. regards, tom lane
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-23T01:50:50Z
On Sat, Jan 22, 2011 at 8:45 PM, Robert Haas <robertmhaas@gmail.com> wrote: >> "Just set" means "use SetConfigOption". > > OK. *reads patch as committed* This is certainly shorter than I wrote, which is good, but it strikes me that the fundamental problem here is that the API for an assign hook is fundamentally different for strings than it is for other data types. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T02:08:22Z
Robert Haas <robertmhaas@gmail.com> writes: > This is certainly shorter than I wrote, which is good, but it strikes > me that the fundamental problem here is that the API for an assign > hook is fundamentally different for strings than it is for other data > types. I agree that that's annoying, but given that strings are pass-by-ref while the other GUC variable types are pass-by-value, it's not really very easy to make them alike. In any case, it's not too relevant to this patch, because an assign hook cannot solve this problem. As someone (I think you) pointed out upthread, an assign hook would only be useful if we were sure wal_buffers would in fact be assigned to by the config file, and that that would happen after shared_buffers acquired its final value. Since we can't assume either thing, the right way to approach it is to have an internal action that assigns a fresh value to wal_buffers after all the configuration processing is complete. Greg had the right design but didn't know how to change a GUC setting properly. There are a bunch of other hacks^Wfeatures that work similarly --- look around for SetConfigOption calls. regards, tom lane
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-23T02:13:07Z
On Sat, Jan 22, 2011 at 9:08 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> This is certainly shorter than I wrote, which is good, but it strikes >> me that the fundamental problem here is that the API for an assign >> hook is fundamentally different for strings than it is for other data >> types. > > I agree that that's annoying, but given that strings are pass-by-ref > while the other GUC variable types are pass-by-value, it's not really > very easy to make them alike. > > In any case, it's not too relevant to this patch, because an assign hook > cannot solve this problem. As someone (I think you) pointed out > upthread, an assign hook would only be useful if we were sure > wal_buffers would in fact be assigned to by the config file, and that > that would happen after shared_buffers acquired its final value. Since > we can't assume either thing, the right way to approach it is to have an > internal action that assigns a fresh value to wal_buffers after all the > configuration processing is complete. Greg had the right design but > didn't know how to change a GUC setting properly. There are a bunch of > other hacks^Wfeatures that work similarly --- look around for > SetConfigOption calls. I'm going with hacks. Any API that requires you to print to a string so you can turn around and immediately convert it back to an integer is not too swift. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T02:24:48Z
Robert Haas <robertmhaas@gmail.com> writes: > I'm going with hacks. Any API that requires you to print to a string > so you can turn around and immediately convert it back to an integer > is not too swift. Oh, you're complaining about SetConfigOption, not the assign hooks. Not sure if it's really worth refactoring that. The problem is that there are lots and lots and lots of places that need to call that *without* any dependency on what the datatype of the target GUC option really is. There are a small number where we do know the type and conversion to a string is just overhead. If any of those were performance-critical it might be worth worrying about, but this one certainly isn't. regards, tom lane
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-23T02:26:42Z
On Sat, Jan 22, 2011 at 9:24 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> I'm going with hacks. Any API that requires you to print to a string >> so you can turn around and immediately convert it back to an integer >> is not too swift. > > Oh, you're complaining about SetConfigOption, not the assign hooks. I was actually complaining about the latter, and then switched gears to the former. I'm an equal-opportunity complainer today, I guess... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T02:42:26Z
Robert Haas <robertmhaas@gmail.com> writes: >> Oh, you're complaining about SetConfigOption, not the assign hooks. > I was actually complaining about the latter, and then switched gears > to the former. I'm an equal-opportunity complainer today, I guess... It does strike me that we could provide SetConfigOptionInt, SetConfigOptionBool, and SetConfigOptionReal for the benefit of callers who'd prefer to pass values in those formats. They'd still do sprintf internally, but this would make the call sites a bit cleaner. In a quick tally, though, I see only a couple of potential callers for SetConfigOptionInt, perhaps a dozen for SetConfigOptionBool, and none at all for SetConfigOptionReal. Hence not sure it's worth the trouble. regards, tom lane
-
Re: auto-sizing wal_buffers
Robert Haas <robertmhaas@gmail.com> — 2011-01-23T02:52:18Z
On Sat, Jan 22, 2011 at 9:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >>> Oh, you're complaining about SetConfigOption, not the assign hooks. > >> I was actually complaining about the latter, and then switched gears >> to the former. I'm an equal-opportunity complainer today, I guess... > > It does strike me that we could provide SetConfigOptionInt, > SetConfigOptionBool, and SetConfigOptionReal for the benefit of callers > who'd prefer to pass values in those formats. They'd still do sprintf > internally, but this would make the call sites a bit cleaner. Why do we need to double the conversion in the first place? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
-
Re: auto-sizing wal_buffers
Tom Lane <tgl@sss.pgh.pa.us> — 2011-01-23T03:12:16Z
Robert Haas <robertmhaas@gmail.com> writes: > On Sat, Jan 22, 2011 at 9:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> It does strike me that we could provide SetConfigOptionInt, >> SetConfigOptionBool, and SetConfigOptionReal for the benefit of callers >> who'd prefer to pass values in those formats. They'd still do sprintf >> internally, but this would make the call sites a bit cleaner. > Why do we need to double the conversion in the first place? Because most of the processing in set_config_option is independent of the type of the GUC variable. Maybe it could be refactored, but I don't think it would come out prettier, nor faster. Again, the important code paths are starting from string values anyway --- I don't think we should contort the design of guc.c to serve a small minority of callers at the expense of complicating the normal cases. regards, tom lane