Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Fabien COELHO]

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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Tom Lane]

Fabien COELHO  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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Fabien COELHO]

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.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);

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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Tomas Vondra]

On 11/25/2016 06:10 PM, Tom Lane wrote:

Fabien COELHO  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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Tom Lane]

Fabien COELHO  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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Tomas Vondra]

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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Fabien COELHO]

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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Tomas Vondra]

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

Re: [HACKERS] confusing checkpoint_flush_after / bgwriter_flush_after

[Fabien COELHO]

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