Re: auto-sizing wal_buffers

On Thu, Jan 6, 2011 at 11:37 PM, Greg Smith <greg(at)2ndquadrant(dot)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

On Thu, Jan 13, 2011 at 23:19, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jan 6, 2011 at 11:37 PM, Greg Smith <greg(at)2ndquadrant(dot)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/

On Thu, Jan 13, 2011 at 5:29 PM, Magnus Hagander <magnus(at)hagander(dot)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

> 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

On Thu, Jan 13, 2011 at 6:02 PM, Josh Berkus <josh(at)agliodbs(dot)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

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

Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 13, 2011 at 5:29 PM, Magnus Hagander <magnus(at)hagander(dot)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

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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

On Sat, Jan 15, 2011 at 3:51 PM, Greg Smith <greg(at)2ndquadrant(dot)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

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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

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

Josh Berkus <josh(at)agliodbs(dot)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

On Sun, Jan 16, 2011 at 00:34, Josh Berkus <josh(at)agliodbs(dot)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

On Sun, Jan 16, 2011 at 1:52 AM, Greg Smith <greg(at)2ndquadrant(dot)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

On Sun, Jan 16, 2011 at 7:34 AM, Josh Berkus <josh(at)agliodbs(dot)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

On Sat, Jan 15, 2011 at 2:34 PM, Josh Berkus <josh(at)agliodbs(dot)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

On Sun, Jan 16, 2011 at 9:32 AM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Josh Berkus <josh(at)agliodbs(dot)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

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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

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(at)2ndQuadrant(dot)com Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support www.2ndQuadrant.us
"PostgreSQL 9.0 High Performance": http://www.2ndQuadrant.com/books

On Tue, Jan 18, 2011 at 8:50 PM, Greg Smith <greg(at)2ndquadrant(dot)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

On Sat, Jan 15, 2011 at 11:52 AM, Greg Smith <greg(at)2ndquadrant(dot)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

On Sat, Jan 22, 2011 at 12:33 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Jan 15, 2011 at 11:52 AM, Greg Smith <greg(at)2ndquadrant(dot)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

On Sat, Jan 22, 2011 at 1:30 AM, Jaime Casanova <jaime(at)2ndquadrant(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

I wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

On Sat, Jan 22, 2011 at 8:33 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Sat, Jan 22, 2011 at 8:45 PM, Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Sat, Jan 22, 2011 at 9:08 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Sat, Jan 22, 2011 at 9:24 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)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

On Sat, Jan 22, 2011 at 9:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Robert Haas <robertmhaas(at)gmail(dot)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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Sat, Jan 22, 2011 at 9:42 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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