Обсуждение: confusing checkpoint_flush_after / bgwriter_flush_after
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
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.
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
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.
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
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
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
>> 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.
Вложения
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
>> 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.