confusing checkpoint_flush_after / bgwriter_flush_after

Started by Tomas Vondraabout 9 years ago10 messages

tomas.vondra@2ndquadrant.com

about 9 years ago

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien COELHO

coelho@cri.ensmp.fr

about 9 years ago

In reply to: Tomas Vondra (#1)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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 disables flushing

--
Fabien.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tomas Vondra

tomas.vondra@2ndquadrant.com

about 9 years ago

In reply to: Fabien COELHO (#2)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien COELHO

coelho@cri.ensmp.fr

about 9 years ago

In reply to: Tomas Vondra (#3)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tomas Vondra

tomas.vondra@2ndquadrant.com

about 9 years ago

In reply to: Fabien COELHO (#4)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tom Lane

tgl@sss.pgh.pa.us

about 9 years ago

In reply to: Fabien COELHO (#2)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Tomas Vondra

tomas.vondra@2ndquadrant.com

about 9 years ago

In reply to: Tom Lane (#6)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Fabien COELHO

coelho@cri.ensmp.fr

about 9 years ago

In reply to: Tomas Vondra (#7)

1 attachment(s)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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.

Attachments:

dbinit-flush-1.patchtext/x-diff; name=dbinit-flush-1.patchDownload

diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index c8a8c52..e014336 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -1107,6 +1107,23 @@ setup_config(void)
 							  "#update_process_title = off");
 #endif
 
+#ifdef HAVE_SYNC_FILE_RANGE
+	/*
+	 * see default in server/storage/bufmgr.h:
+	 * DEFAULT_CHECKPOINT_FLUSH_AFTER 32
+	 * DEFAULT_BGWRITER_FLUSH_AFTER 64
+	 */
+	snprintf(repltok, sizeof(repltok), "#checkpoint_flush_after = %dkb",
+			 32 * (BLCKSZ / 1024));
+	conflines = replace_token(conflines, "#checkpoint_flush_after = 0",
+							  repltok);
+
+	snprintf(repltok, sizeof(repltok), "#bgwriter_flush_after = %dkb",
+			 64 * (BLCKSZ / 1024));
+	conflines = replace_token(conflines, "#bgwriter_flush_after = 0",
+							  repltok);
+#endif   /* HAVE_SYNC_FILE_RANGE */
+
 	snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
 
 	writefile(path, conflines);

Tom Lane

tgl@sss.pgh.pa.us

about 9 years ago

In reply to: Fabien COELHO (#8)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10

Fabien COELHO

coelho@cri.ensmp.fr

about 9 years ago

In reply to: Tom Lane (#9)

Re: confusing checkpoint_flush_after / bgwriter_flush_after

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.

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers