From 22dfabe82b9118aa3c95000f43b9204e2f266b6f Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Tue, 30 Jun 2026 16:18:35 -0400 Subject: [PATCH v1] Improve display of GUCs that are customarily written in octal. A few integer-valued GUCs representing Unix file permission masks are customarily written and shown in octal. However, we implemented the "shown" part via show_hook functions, which is problematic because it only affects display of the current value. It's confusing that, for example, the pg_settings view renders the current value in octal but the min, max, reset, and boot values in decimal. To improve matters, get rid of these custom show_hooks in favor of inventing a per-GUC flag GUC_SHOW_IN_OCTAL, which can be inspected in appropriate places. I've implemented that in places that respond to units flags, but not in places that don't, such as GetConfigOption(). It's tempting to consider going further, in particular adjusting input parsing so that we read the values of these variables in octal even without a leading zero. I've refrained from doing so here because I'm afraid that it'd break more usages than it fixes, but perhaps there's a case to be made for that. Bug: #19540 Reported-by: Jobin Augustine Author: Tom Lane Discussion: https://postgr.es/m/19540-e641040b089ab768@postgresql.org --- src/backend/commands/variable.c | 40 ----------------- src/backend/utils/misc/README | 6 +++ src/backend/utils/misc/guc.c | 53 +++++++++++++++++------ src/backend/utils/misc/guc_funcs.c | 15 +++++-- src/backend/utils/misc/guc_parameters.dat | 7 ++- src/include/utils/guc.h | 1 + src/include/utils/guc_hooks.h | 3 -- src/test/regress/expected/guc.out | 14 ++++++ src/test/regress/sql/guc.sql | 7 +++ 9 files changed, 82 insertions(+), 64 deletions(-) diff --git a/src/backend/commands/variable.c b/src/backend/commands/variable.c index 8afd252fc8c..dd41cd51755 100644 --- a/src/backend/commands/variable.c +++ b/src/backend/commands/variable.c @@ -1171,46 +1171,6 @@ assign_io_combine_limit(int newval, void *extra) io_combine_limit = Min(io_max_combine_limit, newval); } -/* - * These show hooks just exist because we want to show the values in octal. - */ - -/* - * GUC show_hook for data_directory_mode - */ -const char * -show_data_directory_mode(void) -{ - static char buf[12]; - - snprintf(buf, sizeof(buf), "%04o", data_directory_mode); - return buf; -} - -/* - * GUC show_hook for log_file_mode - */ -const char * -show_log_file_mode(void) -{ - static char buf[12]; - - snprintf(buf, sizeof(buf), "%04o", Log_file_mode); - return buf; -} - -/* - * GUC show_hook for unix_socket_permissions - */ -const char * -show_unix_socket_permissions(void) -{ - static char buf[12]; - - snprintf(buf, sizeof(buf), "%04o", Unix_socket_permissions); - return buf; -} - /* * These check hooks do nothing more than reject non-default settings diff --git a/src/backend/utils/misc/README b/src/backend/utils/misc/README index 85d97d29b67..09195e61520 100644 --- a/src/backend/utils/misc/README +++ b/src/backend/utils/misc/README @@ -115,6 +115,12 @@ This hook allows variable-specific computation of the value displayed by SHOW (and other SQL features for showing GUC variable values). The return value can point to a static buffer, since show functions are not used reentrantly. +Note that the show_hook only controls display of the variable's current +value, not its auxiliary values such as min, max, or reset value. +Therefore it is unwise to format the output significantly differently +than the way that those values will be displayed. (As an example, +don't use a show_hook to display a file mode value in octal; instead +set the GUC_SHOW_IN_OCTAL flag.) Saving/Restoring GUC Variable Values diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 774bbc9be5f..21c9ccec1e2 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -3067,20 +3067,37 @@ parse_and_validate_value(const struct config_generic *record, if (newval->intval < conf->min || newval->intval > conf->max) { const char *unit = get_config_unit_name(record->flags); - const char *unitspace; if (unit) - unitspace = " "; + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("%d %s is outside the valid range for parameter \"%s\" (%d %s .. %d %s)", + newval->intval, unit, + record->name, + conf->min, unit, + conf->max, unit))); + } + else if (record->flags & GUC_SHOW_IN_OCTAL) + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("0%03o is outside the valid range for parameter \"%s\" (0%03o .. 0%03o)", + newval->intval, + record->name, + conf->min, + conf->max))); + } else - unit = unitspace = ""; - - ereport(elevel, - (errcode(ERRCODE_INVALID_PARAMETER_VALUE), - errmsg("%d%s%s is outside the valid range for parameter \"%s\" (%d%s%s .. %d%s%s)", - newval->intval, unitspace, unit, - record->name, - conf->min, unitspace, unit, - conf->max, unitspace, unit))); + { + ereport(elevel, + (errcode(ERRCODE_INVALID_PARAMETER_VALUE), + errmsg("%d is outside the valid range for parameter \"%s\" (%d .. %d)", + newval->intval, + record->name, + conf->min, + conf->max))); + } return false; } @@ -5393,7 +5410,7 @@ ShowGUCOption(const struct config_generic *record, bool use_units) if (conf->show_hook) val = conf->show_hook(); - else + else if (use_units && (record->flags & GUC_UNIT)) { /* * Use int64 arithmetic to avoid overflows in units @@ -5402,7 +5419,7 @@ ShowGUCOption(const struct config_generic *record, bool use_units) int64 result = *conf->variable; const char *unit; - if (use_units && result > 0 && (record->flags & GUC_UNIT)) + if (result > 0) convert_int_from_base_unit(result, record->flags & GUC_UNIT, &result, &unit); @@ -5413,6 +5430,16 @@ ShowGUCOption(const struct config_generic *record, bool use_units) result, unit); val = buffer; } + else if (record->flags & GUC_SHOW_IN_OCTAL) + { + snprintf(buffer, sizeof(buffer), "0%03o", *conf->variable); + val = buffer; + } + else + { + snprintf(buffer, sizeof(buffer), "%d", *conf->variable); + val = buffer; + } } break; diff --git a/src/backend/utils/misc/guc_funcs.c b/src/backend/utils/misc/guc_funcs.c index e2c2919484e..0e75c903003 100644 --- a/src/backend/utils/misc/guc_funcs.c +++ b/src/backend/utils/misc/guc_funcs.c @@ -682,24 +682,31 @@ GetConfigOptionValues(const struct config_generic *conf, const char **values) case PGC_INT: { const struct config_int *lconf = &conf->_int; + const char *fmt; + + /* choose display format */ + if (conf->flags & GUC_SHOW_IN_OCTAL) + fmt = "0%03o"; + else + fmt = "%d"; /* min_val */ - snprintf(buffer, sizeof(buffer), "%d", lconf->min); + snprintf(buffer, sizeof(buffer), fmt, lconf->min); values[9] = pstrdup(buffer); /* max_val */ - snprintf(buffer, sizeof(buffer), "%d", lconf->max); + snprintf(buffer, sizeof(buffer), fmt, lconf->max); values[10] = pstrdup(buffer); /* enumvals */ values[11] = NULL; /* boot_val */ - snprintf(buffer, sizeof(buffer), "%d", lconf->boot_val); + snprintf(buffer, sizeof(buffer), fmt, lconf->boot_val); values[12] = pstrdup(buffer); /* reset_val */ - snprintf(buffer, sizeof(buffer), "%d", lconf->reset_val); + snprintf(buffer, sizeof(buffer), fmt, lconf->reset_val); values[13] = pstrdup(buffer); } break; diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index 3c1e6b31bf8..a1d0de57547 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -599,12 +599,11 @@ { name => 'data_directory_mode', type => 'int', context => 'PGC_INTERNAL', group => 'PRESET_OPTIONS', short_desc => 'Shows the mode of the data directory.', long_desc => 'The parameter value is a numeric mode specification in the form accepted by the chmod and umask system calls. (To use the customary octal format the number must start with a 0 (zero).)', - flags => 'GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED', + flags => 'GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE | GUC_RUNTIME_COMPUTED | GUC_SHOW_IN_OCTAL', variable => 'data_directory_mode', boot_val => '0700', min => '0000', max => '0777', - show_hook => 'show_data_directory_mode', }, { name => 'data_sync_retry', type => 'bool', context => 'PGC_POSTMASTER', group => 'ERROR_HANDLING_OPTIONS', @@ -1707,11 +1706,11 @@ { name => 'log_file_mode', type => 'int', context => 'PGC_SIGHUP', group => 'LOGGING_WHERE', short_desc => 'Sets the file permissions for log files.', long_desc => 'The parameter value is expected to be a numeric mode specification in the form accepted by the chmod and umask system calls. (To use the customary octal format the number must start with a 0 (zero).)', + flags => 'GUC_SHOW_IN_OCTAL', variable => 'Log_file_mode', boot_val => '0600', min => '0000', max => '0777', - show_hook => 'show_log_file_mode', }, { name => 'log_filename', type => 'string', context => 'PGC_SIGHUP', group => 'LOGGING_WHERE', @@ -3295,11 +3294,11 @@ { name => 'unix_socket_permissions', type => 'int', context => 'PGC_POSTMASTER', group => 'CONN_AUTH_SETTINGS', short_desc => 'Sets the access permissions of the Unix-domain socket.', long_desc => 'Unix-domain sockets use the usual Unix file system permission set. The parameter value is expected to be a numeric mode specification in the form accepted by the chmod and umask system calls. (To use the customary octal format the number must start with a 0 (zero).)', + flags => 'GUC_SHOW_IN_OCTAL', variable => 'Unix_socket_permissions', boot_val => '0777', min => '0000', max => '0777', - show_hook => 'show_unix_socket_permissions', }, { name => 'update_process_title', type => 'bool', context => 'PGC_SUSET', group => 'PROCESS_TITLE', diff --git a/src/include/utils/guc.h b/src/include/utils/guc.h index dc406d6651a..4ad1aacae67 100644 --- a/src/include/utils/guc.h +++ b/src/include/utils/guc.h @@ -228,6 +228,7 @@ typedef enum 0x002000 /* can't set in PG_AUTOCONF_FILENAME */ #define GUC_RUNTIME_COMPUTED 0x004000 /* delay processing in 'postgres -C' */ #define GUC_ALLOW_IN_PARALLEL 0x008000 /* allow setting in parallel mode */ +#define GUC_SHOW_IN_OCTAL 0x010000 /* show integer variable in octal */ #define GUC_UNIT_KB 0x01000000 /* value is in kilobytes */ #define GUC_UNIT_BLOCKS 0x02000000 /* value is in blocks */ diff --git a/src/include/utils/guc_hooks.h b/src/include/utils/guc_hooks.h index 307f4fbaefe..c2d70e949c9 100644 --- a/src/include/utils/guc_hooks.h +++ b/src/include/utils/guc_hooks.h @@ -46,7 +46,6 @@ extern void assign_client_encoding(const char *newval, void *extra); extern bool check_cluster_name(char **newval, void **extra, GucSource source); extern bool check_commit_ts_buffers(int *newval, void **extra, GucSource source); -extern const char *show_data_directory_mode(void); extern bool check_datestyle(char **newval, void **extra, GucSource source); extern void assign_datestyle(const char *newval, void *extra); extern bool check_debug_io_direct(char **newval, void **extra, GucSource source); @@ -77,7 +76,6 @@ extern void assign_locale_time(const char *newval, void *extra); extern bool check_log_destination(char **newval, void **extra, GucSource source); extern void assign_log_destination(const char *newval, void *extra); -extern const char *show_log_file_mode(void); extern bool check_log_stats(bool *newval, void **extra, GucSource source); extern bool check_log_timezone(char **newval, void **extra, GucSource source); extern void assign_log_timezone(const char *newval, void *extra); @@ -171,7 +169,6 @@ extern bool check_transaction_deferrable(bool *newval, void **extra, GucSource s extern bool check_transaction_isolation(int *newval, void **extra, GucSource source); extern bool check_transaction_read_only(bool *newval, void **extra, GucSource source); extern void assign_transaction_timeout(int newval, void *extra); -extern const char *show_unix_socket_permissions(void); extern bool check_wal_buffers(int *newval, void **extra, GucSource source); extern bool check_wal_consistency_checking(char **newval, void **extra, GucSource source); diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out index 3fa2562f231..0c18fc94e31 100644 --- a/src/test/regress/expected/guc.out +++ b/src/test/regress/expected/guc.out @@ -928,6 +928,20 @@ SELECT query_id IS NOT NULL AS qid_set FROM pg_stat_activity RESET track_activities; RESET compute_query_id; +-- Test GUC_SHOW_IN_OCTAL +SELECT name, min_val, max_val, boot_val +FROM pg_settings +WHERE name IN ('data_directory_mode', 'log_file_mode', + 'unix_socket_permissions'); + name | min_val | max_val | boot_val +-------------------------+---------+---------+---------- + data_directory_mode | 0000 | 0777 | 0700 + log_file_mode | 0000 | 0777 | 0600 + unix_socket_permissions | 0000 | 0777 | 0777 +(3 rows) + +ALTER SYSTEM SET log_file_mode = 0640; -- decimal, so out of range +ERROR: 01200 is outside the valid range for parameter "log_file_mode" (0000 .. 0777) -- Test GUC categories and flag patterns SELECT pg_settings_get_flags(NULL); pg_settings_get_flags diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql index dfb843fd3ae..e78b4af3a3a 100644 --- a/src/test/regress/sql/guc.sql +++ b/src/test/regress/sql/guc.sql @@ -365,6 +365,13 @@ SELECT query_id IS NOT NULL AS qid_set FROM pg_stat_activity RESET track_activities; RESET compute_query_id; +-- Test GUC_SHOW_IN_OCTAL +SELECT name, min_val, max_val, boot_val +FROM pg_settings +WHERE name IN ('data_directory_mode', 'log_file_mode', + 'unix_socket_permissions'); +ALTER SYSTEM SET log_file_mode = 0640; -- decimal, so out of range + -- Test GUC categories and flag patterns SELECT pg_settings_get_flags(NULL); SELECT pg_settings_get_flags('does_not_exist'); -- 2.52.0