Thread: confusing checkpoint_flush_after / bgwriter_flush_after

confusing checkpoint_flush_after / bgwriter_flush_after

From
Tomas Vondra
Date:
Hi,

while doing some benchmarking, I've once again got confused by the 
default settings for checkpoint_flush_after and bgwriter_flush_after. 
The sample config says this:

#checkpoint_flush_after = 0   # 0 disables,                              # default is 256kB on linux, 0 otherwise

#bgwriter_flush_after = 0     # 0 disables,                              # default is 512kB on linux, 0 otherwise

I find this pretty confusing, because for all other GUCs in the file, 
the commented-out value is the default one. In this case that would mean 
"0", disabling the flushing.

But in practice we use platform-dependent defaults - 256/512K on Linux, 
0 otherwise. There are other GUCs where the default is 
platform-specific, but none of them suggests "disabled" is the default 
state.

While the 9.6 cat is out of the bag, I think we can fix this quite 
easily - use "-1" to specify the default value should be used, and use 
that in the sample file. This won't break any user configuration.

If that's considered not acceptable, perhaps we should at least improve 
the comments, so make this clearer.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Fabien COELHO
Date:
Hello Tomas,

> #checkpoint_flush_after = 0   # 0 disables,
>                              # default is 256kB on linux, 0 otherwise

> I find this pretty confusing, because for all other GUCs in the file, the 
> commented-out value is the default one. In this case that would mean "0", 
> disabling the flushing.
>
> But in practice we use platform-dependent defaults - 256/512K on Linux, 0 
> otherwise. There are other GUCs where the default is platform-specific, but 
> none of them suggests "disabled" is the default state.
>
> While the 9.6 cat is out of the bag, I think we can fix this quite easily - 
> use "-1" to specify the default value should be used, and use that in the 
> sample file. This won't break any user configuration.

Although I understand the issue, I'm not sure about -1 as a special value 
to mean the default.

> If that's considered not acceptable, perhaps we should at least improve the 
> comments, so make this clearer.

Yep, what about not putting a value and inverting/adapting the comments, 
maybe something like:
 #checkpoint_flush_after = ...  # default is 256kB on linux, 0 otherwise                                # where 0
disablesflushing
 


-- 
Fabien.



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Tomas Vondra
Date:
On 11/25/2016 01:20 PM, Fabien COELHO wrote:
>
> Hello Tomas,
>
>> #checkpoint_flush_after = 0   # 0 disables,
>>                              # default is 256kB on linux, 0 otherwise
>
>> I find this pretty confusing, because for all other GUCs in the file,
>> the commented-out value is the default one. In this case that would
>> mean "0", disabling the flushing.
>>
>> But in practice we use platform-dependent defaults - 256/512K on
>> Linux, 0 otherwise. There are other GUCs where the default is
>> platform-specific, but none of them suggests "disabled" is the default
>> state.
>>
>> While the 9.6 cat is out of the bag, I think we can fix this quite
>> easily - use "-1" to specify the default value should be used, and use
>> that in the sample file. This won't break any user configuration.
>
> Although I understand the issue, I'm not sure about -1 as a special
> value to mean the default.
>

Why? We use wal_buffers=-1 to use the default (depending on the size of 
shared_buffers), for example.

>> If that's considered not acceptable, perhaps we should at least
>> improve the comments, so make this clearer.
>
> Yep, what about not putting a value and inverting/adapting the comments,
> maybe something like:
>
>  #checkpoint_flush_after = ...  # default is 256kB on linux, 0 otherwise
>                                 # where 0 disables flushing
>

Yeah, something like that.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Fabien COELHO
Date:
Hello Tomas,

>>> While the 9.6 cat is out of the bag, I think we can fix this quite
>>> easily - use "-1" to specify the default value should be used, and use
>>> that in the sample file. This won't break any user configuration.
>> 
>> Although I understand the issue, I'm not sure about -1 as a special
>> value to mean the default.
>
> Why? We use wal_buffers=-1 to use the default (depending on the size of 
> shared_buffers), for example.

Indeed. Just my 0.02€:

ISTM that the use of -1 is not very consistent, as it may mean:
 - default: autovacuum_work_mem, wal_buffers
 - disable: temp_file_limit, old_snapshot_limit,     max_standby_*_delay, log_min_duration_statement

And sometimes disable is the default, but not always:-) Basically I'm not 
sure that adding some more confusion around that helps much...

-- 
Fabien.

Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Tomas Vondra
Date:
On 11/25/2016 04:40 PM, Fabien COELHO wrote:
>
> Hello Tomas,
>
>>>> While the 9.6 cat is out of the bag, I think we can fix this quite
>>>> easily - use "-1" to specify the default value should be used, and use
>>>> that in the sample file. This won't break any user configuration.
>>>
>>> Although I understand the issue, I'm not sure about -1 as a special
>>> value to mean the default.
>>
>> Why? We use wal_buffers=-1 to use the default (depending on the size
>> of shared_buffers), for example.
>
> Indeed. Just my 0.02€:
>
> ISTM that the use of -1 is not very consistent, as it may mean:
>
>  - default: autovacuum_work_mem, wal_buffers
>
>  - disable: temp_file_limit, old_snapshot_limit,
>      max_standby_*_delay, log_min_duration_statement
>
> And sometimes disable is the default, but not always:-) Basically I'm
> not sure that adding some more confusion around that helps much...
>

Well, the inconsistency is already there. Some GUCs use -1 as "use 
default value" and others using it as "disable". Picking one of those 
does not really increase the confusion, and it fixes the issue of having 
a default mismatching the commented-out example.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Tom Lane
Date:
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>> #checkpoint_flush_after = 0   # 0 disables,
>>                               # default is 256kB on linux, 0 otherwise

>> I find this pretty confusing, because for all other GUCs in the file, the 
>> commented-out value is the default one. In this case that would mean "0", 
>> disabling the flushing.

> Although I understand the issue, I'm not sure about -1 as a special value 
> to mean the default.

Agreed --- I think that's making things more confusing not less.

What we do in some similar cases is put the burden on initdb to fill in
the correct value by modifying postgresql.conf.sample appropriately.
It seems like that could be done easily here too.  And it'd be a
back-patchable fix.
        regards, tom lane



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Tomas Vondra
Date:
On 11/25/2016 06:10 PM, Tom Lane wrote:
> Fabien COELHO <coelho@cri.ensmp.fr> writes:
>>> #checkpoint_flush_after = 0   # 0 disables,
>>>                               # default is 256kB on linux, 0 otherwise
>
>>> I find this pretty confusing, because for all other GUCs in the file, the
>>> commented-out value is the default one. In this case that would mean "0",
>>> disabling the flushing.
>
>> Although I understand the issue, I'm not sure about -1 as a special value
>> to mean the default.
>
> Agreed --- I think that's making things more confusing not less.
>
> What we do in some similar cases is put the burden on initdb to fill in
> the correct value by modifying postgresql.conf.sample appropriately.
> It seems like that could be done easily here too.  And it'd be a
> back-patchable fix.
>

I haven't realized initdb can do that. I agree that would be the best 
solution.


-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Fabien COELHO
Date:
>> What we do in some similar cases is put the burden on initdb to fill in
>> the correct value by modifying postgresql.conf.sample appropriately.
>> It seems like that could be done easily here too.  And it'd be a
>> back-patchable fix.
>
> I haven't realized initdb can do that. I agree that would be the best
> solution.

Indeed.

Maybe something like the following, or maybe it should include "bufmgr.h",
not sure.

--
Fabien.
Attachment

Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Tom Lane
Date:
Fabien COELHO <coelho@cri.ensmp.fr> writes:
>>> What we do in some similar cases is put the burden on initdb to fill in
>>> the correct value by modifying postgresql.conf.sample appropriately.
>>> It seems like that could be done easily here too.  And it'd be a
>>> back-patchable fix.

>> I haven't realized initdb can do that. I agree that would be the best 
>> solution.

> Indeed.

> Maybe something like the following, or maybe it should include "bufmgr.h", 
> not sure.

As-is this patch seems like a maintenance time bomb; it really needs to
use the #defines rather than have the values hard-wired in.  However, just
including bufmgr.h in frontend code doesn't work, so I moved the #defines
to pg_config_manual.h, which seems like a more reasonable place for them
anyway.

Pushed with that and some other polishing.
        regards, tom lane



Re: confusing checkpoint_flush_after / bgwriter_flush_after

From
Fabien COELHO
Date:
>> Maybe something like the following, or maybe it should include "bufmgr.h",
>> not sure.
>
> As-is this patch seems like a maintenance time bomb; it really needs to
> use the #defines rather than have the values hard-wired in.  However, just
> including bufmgr.h in frontend code doesn't work, so I moved the #defines
> to pg_config_manual.h, which seems like a more reasonable place for them
> anyway. Pushed with that and some other polishing.

Indeed, that's much cleaner and easier to maintain. Thanks.

-- 
Fabien.