On Mon, Nov 29, 2021 at 05:04:01PM +0900, Michael Paquier wrote: > 0001, to adjust the units, and 0003, to make the GUC descriptions less > unit-dependent, are good ideas.
Actually, after sleeping on it and doing some digging in codesearch.debian.org, changing the units of max_identifier_length, block_size and wal_block_size could induce some breakages for anything using a SHOW command, something that becomes particularly worse now that SHOW is supported in replication connections, and it would force clients to know and parse the units of a value. Perhaps I am being too careful here, but that could harm a lot of users. It is worth noting that I have found some driver code making use of pg_settings, which would not be influenced by such a change, but it is unsafe to assume that everybody does that. The addition of GUC_EXPLAIN for enable_incremental_sort, the comment fix for autovacuum_freeze_max_age, the use of COMPAT_OPTIONS_PREVIOUS for ssl_renegotiation_limit and the addition of GUC_NOT_IN_SAMPLE for trace_recovery_messages are fine, though. > I am not completely sure that all the contents of 0002 are > improvements, but the suggestions done for huge_pages, > ssl_passphrase_command_supports_reload, checkpoint_warning and > commit_siblings seem fine to me. Hmm, I think the patched description of checkpoint_warning is not that much an improvement compared to the current one. While the current description uses the term "checkpoint segments", which is, I agree, weird. The new one would lose the term "checkpoint", making the short description of the parameter lose some of its context. I have done a full review of the patch set, and applied the obvious fixes/improvements as of be54551. Attached is an extra patch based on the contents of the whole set sent upthread: - Improvement of the description of checkpoint_segments. - Reworded the description of all parameters using "N units", rather than just switching to "this period of units". I have been using something more generic. Thoughts? -- Michael
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 25ac4ca85b..927212b8ae 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -2131,8 +2131,8 @@ static struct config_int ConfigureNamesInt[] =
{
{
{"archive_timeout", PGC_SIGHUP, WAL_ARCHIVING,
- gettext_noop("Forces a switch to the next WAL file if a "
- "new file has not been started within N seconds."),
+ gettext_noop("Sets the amount of time to wait before forcing a "
+ "switch to the next WAL file."),
NULL,
GUC_UNIT_S
},
@@ -2142,7 +2142,8 @@ static struct config_int ConfigureNamesInt[] =
},
{
{"post_auth_delay", PGC_BACKEND, DEVELOPER_OPTIONS,
- gettext_noop("Waits N seconds on connection startup after authentication."),
+ gettext_noop("Sets the amount of seconds to wait on connection "
+ "startup after authentication."),
gettext_noop("This allows attaching a debugger to the process."),
GUC_NOT_IN_SAMPLE | GUC_UNIT_S
},
@@ -2762,7 +2763,8 @@ static struct config_int ConfigureNamesInt[] =
{
/* Not for general use */
{"pre_auth_delay", PGC_SIGHUP, DEVELOPER_OPTIONS,
- gettext_noop("Waits N seconds on connection startup before authentication."),
+ gettext_noop("Sets the amount of seconds to wait on connection "
+ "startup before authentication."),
gettext_noop("This allows attaching a debugger to the process."),
GUC_NOT_IN_SAMPLE | GUC_UNIT_S
},
@@ -2819,10 +2821,10 @@ static struct config_int ConfigureNamesInt[] =
{
{"checkpoint_warning", PGC_SIGHUP, WAL_CHECKPOINTS,
- gettext_noop("Enables warnings if checkpoint segments are filled more "
- "frequently than this."),
+ gettext_noop("Sets the maximum time before warning if checkpoints "
+ "triggered by WAL volume happen too frequently."),
gettext_noop("Write a message to the server log if checkpoints "
- "caused by the filling of checkpoint segment files happens more "
+ "caused by the filling of WAL segment files happens more "
"frequently than this number of seconds. Zero turns off the warning."),
GUC_UNIT_S
},
@@ -3006,7 +3008,8 @@ static struct config_int ConfigureNamesInt[] =
{
{"log_parameter_max_length", PGC_SUSET, LOGGING_WHAT,
- gettext_noop("When logging statements, limit logged parameter values to first N bytes."),
+ gettext_noop("Sets the maximum amount of data logged for bind "
+ "parameter values when logging statements."),
gettext_noop("-1 to print values in full."),
GUC_UNIT_BYTE
},
@@ -3017,7 +3020,8 @@ static struct config_int ConfigureNamesInt[] =
{
{"log_parameter_max_length_on_error", PGC_USERSET, LOGGING_WHAT,
- gettext_noop("When reporting an error, limit logged parameter values to first N bytes."),
+ gettext_noop("Sets the maximum amount of data logged for bind "
+ "parameter values when logging statements, on error."),
gettext_noop("-1 to print values in full."),
GUC_UNIT_BYTE
},
@@ -3143,7 +3147,8 @@ static struct config_int ConfigureNamesInt[] =
{
{"log_rotation_age", PGC_SIGHUP, LOGGING_WHERE,
- gettext_noop("Automatic log file rotation will occur after N minutes."),
+ gettext_noop("Sets the maximum amount of time to wait before "
+ "forcing log file rotation."),
NULL,
GUC_UNIT_MIN
},
@@ -3154,7 +3159,8 @@ static struct config_int ConfigureNamesInt[] =
{
{"log_rotation_size", PGC_SIGHUP, LOGGING_WHERE,
- gettext_noop("Automatic log file rotation will occur after N kilobytes."),
+ gettext_noop("Sets the maximum size of log file to reach before "
+ "forcing log file rotation."),
NULL,
GUC_UNIT_KB
},
signature.asc
Description: PGP signature
