Обсуждение: [PATCH] Proof of concept for GUC improvements
-hackers,
Enclosed, find a POC patch that implements "special values" for int GUCs. We have quite a few GUCs
with values that have special meaning atop other settings. I have attempted to identify these and
make it so you can specify a symbol name for these values instead of just relying on the magic
number instead.
For instance, many GUCs have -1 for "disabled", so I've just made it so you can
specify something like:
SET log_min_duration_statement = disabled;
And the raw value will be set to -1 in this case. For the purposes of testing, I have also added a
new GUC "output_special_values" to affect whether `SHOW` or anything that relies on _ShowOption()
can show with the special value instead of just the raw magic value, allowing tools to consume the
original raw value, or provide the output to the user in the nicer format.
This has only been done for ints, and the passthru I did was very quick, so I have probably missed
some options that didn't explicitly have their interpretations in the file and/or I didn't know
about it already. I do not think there are these sorts of values in other non-int GUCs, but there
might be, so a similar approach could be taken to expand things to other config types in the future.
Let me know your thoughts; I personally find this to be useful, and would be a nicer way for some
configs to be displayed in the postgresql.conf file.
Best,
David
From 68aef8f20d495bee14bbf27808beadecfb50612a Mon Sep 17 00:00:00 2001
From: David Christensen <david.christensen@crunchydata.com>
Date: Thu, 19 Aug 2021 14:33:15 -0500
Subject: [PATCH] POC: use sentinel values for parsing/outputting "special" int
GUCs
Some GUCs include specific key values like "-1", etc, which have context beyond the actual value
involved. Add support for providing synonyms for these values on input/output which make things a
lot easier to understand.
Add a new GUC output_special_values to enable the change of behavior on output; by default we leave
this alone, as there may be tools parsing/using these values as they currently output.
As far as I know, this "magic values" really only exists for ints; we can expand things similarly
for other types if the need arises.
For now, a non-exhaustive pass has been done through the config_int options list to identify
likely/needed values. This will probably need to be tightened up in the future.
This code supports multiple values; it will stop at the first parsed found value and use that int
value.
---
src/backend/utils/misc/guc.c | 374 ++++++++++++++++++++----------
src/include/utils/guc_tables.h | 1 +
src/test/regress/expected/guc.out | 45 ++++
src/test/regress/sql/guc.sql | 16 ++
4 files changed, 308 insertions(+), 128 deletions(-)
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index a2e0f8de7e..f2f7f55ed7 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -235,6 +235,9 @@ static void assign_recovery_target_lsn(const char *newval, void *extra);
static bool check_primary_slot_name(char **newval, void **extra, GucSource source);
static bool check_default_with_oids(bool *newval, void **extra, GucSource source);
+static bool parse_special_int(const struct config_enum_entry *options, const char *value, int *result);
+static bool special_int_to_value(const struct config_enum_entry *options, int value, const char **retval);
+
/* Private functions in guc-file.l that need to be called from guc.c */
static ConfigVariable *ProcessConfigFileInternal(GucContext context,
bool applySettings, int elevel);
@@ -565,6 +568,57 @@ extern const struct config_enum_entry recovery_target_action_options[];
extern const struct config_enum_entry sync_method_options[];
extern const struct config_enum_entry dynamic_shared_memory_options[];
+/* Some static structs for use in options which have -1 as special values;
+ * example: "disabled" or "inherited". While these are not enums per se, we
+ * are reusing the struct, with the bool field indicating whether to print the
+ * translated values on output */
+
+static const struct config_enum_entry special_auto[] = {
+ {"auto", -1, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_disabled[] = {
+ {"disabled", -1, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_disabled0[] = {
+ {"disabled", 0, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_disabled_all[] = {
+ {"disabled", -1, false},
+ {"all", 0, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_default0[] = {
+ {"default", 0, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_immediate0[] = {
+ {"immediate", 0, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_none0[] = {
+ {"none", 0, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_unlimited[] = {
+ {"unlimited", -1, false},
+ {NULL, 0, false}
+};
+
+static const struct config_enum_entry special_unlimited0[] = {
+ {"unlimited", 0, false},
+ {NULL, 0, false}
+};
+
/*
* GUC option variables that are exported from this module
*/
@@ -592,6 +646,8 @@ bool check_function_bodies = true;
bool default_with_oids = false;
bool session_auth_is_superuser;
+bool output_special_values = false;
+
int log_min_error_statement = ERROR;
int log_min_messages = WARNING;
int client_min_messages = NOTICE;
@@ -2116,6 +2172,15 @@ static struct config_bool ConfigureNamesBool[] =
NULL, NULL, NULL
},
+ {
+ {"output_special_values", PGC_USERSET, CLIENT_CONN_OTHER,
+ gettext_noop("Whether to display \"special\" values in settings display."),
+ },
+ &output_special_values,
+ false,
+ NULL, NULL, NULL
+ },
+
/* End-of-list marker */
{
{NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL
@@ -2134,7 +2199,7 @@ static struct config_int ConfigureNamesInt[] =
},
&XLogArchiveTimeout,
0, 0, INT_MAX / 2,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
{"post_auth_delay", PGC_BACKEND, DEVELOPER_OPTIONS,
@@ -2144,7 +2209,7 @@ static struct config_int ConfigureNamesInt[] =
},
&PostAuthDelay,
0, 0, INT_MAX / 1000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
{"default_statistics_target", PGC_USERSET, QUERY_TUNING_OTHER,
@@ -2154,7 +2219,7 @@ static struct config_int ConfigureNamesInt[] =
},
&default_statistics_target,
100, 1, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"from_collapse_limit", PGC_USERSET, QUERY_TUNING_OTHER,
@@ -2167,7 +2232,7 @@ static struct config_int ConfigureNamesInt[] =
},
&from_collapse_limit,
8, 1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"join_collapse_limit", PGC_USERSET, QUERY_TUNING_OTHER,
@@ -2180,7 +2245,7 @@ static struct config_int ConfigureNamesInt[] =
},
&join_collapse_limit,
8, 1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"geqo_threshold", PGC_USERSET, QUERY_TUNING_GEQO,
@@ -2190,7 +2255,7 @@ static struct config_int ConfigureNamesInt[] =
},
&geqo_threshold,
12, 2, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"geqo_effort", PGC_USERSET, QUERY_TUNING_GEQO,
@@ -2200,7 +2265,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Geqo_effort,
DEFAULT_GEQO_EFFORT, MIN_GEQO_EFFORT, MAX_GEQO_EFFORT,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"geqo_pool_size", PGC_USERSET, QUERY_TUNING_GEQO,
@@ -2210,7 +2275,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Geqo_pool_size,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"geqo_generations", PGC_USERSET, QUERY_TUNING_GEQO,
@@ -2220,7 +2285,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Geqo_generations,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2232,7 +2297,7 @@ static struct config_int ConfigureNamesInt[] =
},
&DeadlockTimeout,
1000, 1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2243,7 +2308,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_standby_archive_delay,
30 * 1000, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited
},
{
@@ -2254,7 +2319,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_standby_streaming_delay,
30 * 1000, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited
},
{
@@ -2265,7 +2330,7 @@ static struct config_int ConfigureNamesInt[] =
},
&recovery_min_apply_delay,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2276,7 +2341,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_receiver_status_interval,
10, 0, INT_MAX / 1000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -2287,7 +2352,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_receiver_timeout,
60 * 1000, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -2297,7 +2362,7 @@ static struct config_int ConfigureNamesInt[] =
},
&MaxConnections,
100, 1, MAX_BACKENDS,
- check_maxconnections, NULL, NULL
+ check_maxconnections, NULL, NULL, NULL
},
{
@@ -2308,7 +2373,7 @@ static struct config_int ConfigureNamesInt[] =
},
&ReservedBackends,
3, 0, MAX_BACKENDS,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2319,7 +2384,7 @@ static struct config_int ConfigureNamesInt[] =
},
&min_dynamic_shared_memory,
0, 0, (int) Min((size_t) INT_MAX, SIZE_MAX / (1024 * 1024)),
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
/*
@@ -2334,7 +2399,7 @@ static struct config_int ConfigureNamesInt[] =
},
&NBuffers,
1024, 16, INT_MAX / 2,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2345,7 +2410,7 @@ static struct config_int ConfigureNamesInt[] =
},
&num_temp_buffers,
1024, 100, INT_MAX / 2,
- check_temp_buffers, NULL, NULL
+ check_temp_buffers, NULL, NULL, NULL
},
{
@@ -2355,7 +2420,7 @@ static struct config_int ConfigureNamesInt[] =
},
&PostPortNumber,
DEF_PGPORT, 1, 65535,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2370,7 +2435,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Unix_socket_permissions,
0777, 0000, 0777,
- NULL, NULL, show_unix_socket_permissions
+ NULL, NULL, show_unix_socket_permissions, NULL
},
{
@@ -2384,7 +2449,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Log_file_mode,
0600, 0000, 0777,
- NULL, NULL, show_log_file_mode
+ NULL, NULL, show_log_file_mode, NULL
},
@@ -2399,7 +2464,7 @@ static struct config_int ConfigureNamesInt[] =
},
&data_directory_mode,
0700, 0000, 0777,
- NULL, NULL, show_data_directory_mode
+ NULL, NULL, show_data_directory_mode, NULL
},
{
@@ -2412,7 +2477,7 @@ static struct config_int ConfigureNamesInt[] =
},
&work_mem,
4096, 64, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2423,7 +2488,7 @@ static struct config_int ConfigureNamesInt[] =
},
&maintenance_work_mem,
65536, 1024, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2435,7 +2500,7 @@ static struct config_int ConfigureNamesInt[] =
},
&logical_decoding_work_mem,
65536, 64, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
/*
@@ -2451,7 +2516,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_stack_depth,
100, 100, MAX_KILOBYTES,
- check_max_stack_depth, assign_max_stack_depth, NULL
+ check_max_stack_depth, assign_max_stack_depth, NULL, NULL
},
{
@@ -2462,7 +2527,7 @@ static struct config_int ConfigureNamesInt[] =
},
&temp_file_limit,
-1, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited
},
{
@@ -2472,7 +2537,7 @@ static struct config_int ConfigureNamesInt[] =
},
&VacuumCostPageHit,
1, 0, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2482,7 +2547,7 @@ static struct config_int ConfigureNamesInt[] =
},
&VacuumCostPageMiss,
2, 0, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2492,7 +2557,7 @@ static struct config_int ConfigureNamesInt[] =
},
&VacuumCostPageDirty,
20, 0, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2502,7 +2567,7 @@ static struct config_int ConfigureNamesInt[] =
},
&VacuumCostLimit,
200, 1, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2512,7 +2577,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_vac_cost_limit,
-1, -1, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2522,7 +2587,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_files_per_process,
1000, 64, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
/*
@@ -2535,7 +2600,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_prepared_xacts,
0, 0, MAX_BACKENDS,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
#ifdef LOCK_DEBUG
@@ -2547,7 +2612,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Trace_lock_oidmin,
FirstNormalObjectId, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"trace_lock_table", PGC_SUSET, DEVELOPER_OPTIONS,
@@ -2557,7 +2622,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Trace_lock_table,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
#endif
@@ -2569,7 +2634,7 @@ static struct config_int ConfigureNamesInt[] =
},
&StatementTimeout,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited0
},
{
@@ -2580,7 +2645,7 @@ static struct config_int ConfigureNamesInt[] =
},
&LockTimeout,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited0
},
{
@@ -2591,7 +2656,7 @@ static struct config_int ConfigureNamesInt[] =
},
&IdleInTransactionSessionTimeout,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited0
},
{
@@ -2602,7 +2667,7 @@ static struct config_int ConfigureNamesInt[] =
},
&IdleSessionTimeout,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited0
},
{
@@ -2612,7 +2677,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_freeze_min_age,
50000000, 0, 1000000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2622,7 +2687,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_freeze_table_age,
150000000, 0, 2000000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2632,7 +2697,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_multixact_freeze_min_age,
5000000, 0, 1000000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2642,7 +2707,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_multixact_freeze_table_age,
150000000, 0, 2000000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2652,7 +2717,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_defer_cleanup_age,
0, 0, 1000000, /* see ComputeXidHorizons */
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"vacuum_failsafe_age", PGC_USERSET, CLIENT_CONN_STATEMENT,
@@ -2661,7 +2726,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_failsafe_age,
1600000000, 0, 2100000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"vacuum_multixact_failsafe_age", PGC_USERSET, CLIENT_CONN_STATEMENT,
@@ -2670,7 +2735,7 @@ static struct config_int ConfigureNamesInt[] =
},
&vacuum_multixact_failsafe_age,
1600000000, 0, 2100000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
/*
@@ -2685,7 +2750,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_locks_per_xact,
64, 10, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2697,7 +2762,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_predicate_locks_per_xact,
64, 10, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2708,7 +2773,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_predicate_locks_per_relation,
-2, INT_MIN, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2719,7 +2784,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_predicate_locks_per_page,
2, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2730,7 +2795,7 @@ static struct config_int ConfigureNamesInt[] =
},
&AuthenticationTimeout,
60, 1, 600,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2742,7 +2807,7 @@ static struct config_int ConfigureNamesInt[] =
},
&PreAuthDelay,
0, 0, 60,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -2753,7 +2818,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_keep_size_mb,
0, 0, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_none0
},
{
@@ -2765,7 +2830,7 @@ static struct config_int ConfigureNamesInt[] =
&min_wal_size_mb,
DEFAULT_MIN_WAL_SEGS * (DEFAULT_XLOG_SEG_SIZE / (1024 * 1024)),
2, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2777,7 +2842,7 @@ static struct config_int ConfigureNamesInt[] =
&max_wal_size_mb,
DEFAULT_MAX_WAL_SEGS * (DEFAULT_XLOG_SEG_SIZE / (1024 * 1024)),
2, MAX_KILOBYTES,
- NULL, assign_max_wal_size, NULL
+ NULL, assign_max_wal_size, NULL, NULL
},
{
@@ -2788,7 +2853,7 @@ static struct config_int ConfigureNamesInt[] =
},
&CheckPointTimeout,
300, 30, 86400,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2802,7 +2867,7 @@ static struct config_int ConfigureNamesInt[] =
},
&CheckPointWarning,
30, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -2813,7 +2878,7 @@ static struct config_int ConfigureNamesInt[] =
},
&checkpoint_flush_after,
DEFAULT_CHECKPOINT_FLUSH_AFTER, 0, WRITEBACK_MAX_PENDING_FLUSHES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -2824,7 +2889,7 @@ static struct config_int ConfigureNamesInt[] =
},
&XLOGbuffers,
-1, -1, (INT_MAX / XLOG_BLCKSZ),
- check_wal_buffers, NULL, NULL
+ check_wal_buffers, NULL, NULL, special_auto
},
{
@@ -2835,7 +2900,7 @@ static struct config_int ConfigureNamesInt[] =
},
&WalWriterDelay,
200, 1, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2846,7 +2911,7 @@ static struct config_int ConfigureNamesInt[] =
},
&WalWriterFlushAfter,
(1024 * 1024) / XLOG_BLCKSZ, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_immediate0
},
{
@@ -2857,7 +2922,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_skip_threshold,
2048, 0, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2867,7 +2932,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_wal_senders,
10, 0, MAX_BACKENDS,
- check_max_wal_senders, NULL, NULL
+ check_max_wal_senders, NULL, NULL, NULL
},
{
@@ -2878,7 +2943,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_replication_slots,
10, 0, MAX_BACKENDS /* XXX? */ ,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2891,7 +2956,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_slot_wal_keep_size_mb,
-1, -1, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited
},
{
@@ -2902,7 +2967,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_sender_timeout,
60 * 1000, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -2914,7 +2979,7 @@ static struct config_int ConfigureNamesInt[] =
},
&CommitDelay,
0, 0, 100000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_none0
},
{
@@ -2925,7 +2990,7 @@ static struct config_int ConfigureNamesInt[] =
},
&CommitSiblings,
5, 0, 1000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2938,7 +3003,7 @@ static struct config_int ConfigureNamesInt[] =
},
&extra_float_digits,
1, -15, 3,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -2951,7 +3016,7 @@ static struct config_int ConfigureNamesInt[] =
},
&log_min_duration_sample,
-1, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled_all
},
{
@@ -2963,7 +3028,7 @@ static struct config_int ConfigureNamesInt[] =
},
&log_min_duration_statement,
-1, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled_all
},
{
@@ -2975,7 +3040,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Log_autovacuum_min_duration,
-1, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled_all
},
{
@@ -2986,7 +3051,7 @@ static struct config_int ConfigureNamesInt[] =
},
&log_parameter_max_length,
-1, -1, INT_MAX / 2,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited
},
{
@@ -2997,7 +3062,7 @@ static struct config_int ConfigureNamesInt[] =
},
&log_parameter_max_length_on_error,
0, -1, INT_MAX / 2,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_unlimited
},
{
@@ -3008,7 +3073,7 @@ static struct config_int ConfigureNamesInt[] =
},
&BgWriterDelay,
200, 10, 10000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3018,7 +3083,7 @@ static struct config_int ConfigureNamesInt[] =
},
&bgwriter_lru_maxpages,
100, 0, INT_MAX / 2, /* Same upper limit as shared_buffers */
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3029,7 +3094,7 @@ static struct config_int ConfigureNamesInt[] =
},
&bgwriter_flush_after,
DEFAULT_BGWRITER_FLUSH_AFTER, 0, WRITEBACK_MAX_PENDING_FLUSHES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -3047,7 +3112,7 @@ static struct config_int ConfigureNamesInt[] =
0,
#endif
0, MAX_IO_CONCURRENCY,
- check_effective_io_concurrency, NULL, NULL
+ check_effective_io_concurrency, NULL, NULL, NULL
},
{
@@ -3065,7 +3130,7 @@ static struct config_int ConfigureNamesInt[] =
0,
#endif
0, MAX_IO_CONCURRENCY,
- check_maintenance_io_concurrency, NULL, NULL
+ check_maintenance_io_concurrency, NULL, NULL, NULL
},
{
@@ -3076,7 +3141,7 @@ static struct config_int ConfigureNamesInt[] =
},
&backend_flush_after,
DEFAULT_BACKEND_FLUSH_AFTER, 0, WRITEBACK_MAX_PENDING_FLUSHES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled0
},
{
@@ -3088,7 +3153,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_worker_processes,
8, 0, MAX_BACKENDS,
- check_max_worker_processes, NULL, NULL
+ check_max_worker_processes, NULL, NULL, NULL
},
{
@@ -3100,7 +3165,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_logical_replication_workers,
4, 0, MAX_BACKENDS,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3112,7 +3177,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_sync_workers_per_subscription,
2, 0, MAX_BACKENDS,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3123,7 +3188,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Log_RotationAge,
HOURS_PER_DAY * MINS_PER_HOUR, 0, INT_MAX / SECS_PER_MINUTE,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3134,7 +3199,7 @@ static struct config_int ConfigureNamesInt[] =
},
&Log_RotationSize,
10 * 1024, 0, INT_MAX / 1024,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3145,7 +3210,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_function_args,
FUNC_MAX_ARGS, FUNC_MAX_ARGS, FUNC_MAX_ARGS,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3156,7 +3221,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_index_keys,
INDEX_MAX_KEYS, INDEX_MAX_KEYS, INDEX_MAX_KEYS,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3167,7 +3232,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_identifier_length,
NAMEDATALEN - 1, NAMEDATALEN - 1, NAMEDATALEN - 1,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3178,7 +3243,7 @@ static struct config_int ConfigureNamesInt[] =
},
&block_size,
BLCKSZ, BLCKSZ, BLCKSZ,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3189,7 +3254,7 @@ static struct config_int ConfigureNamesInt[] =
},
&segment_size,
RELSEG_SIZE, RELSEG_SIZE, RELSEG_SIZE,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3200,7 +3265,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_block_size,
XLOG_BLCKSZ, XLOG_BLCKSZ, XLOG_BLCKSZ,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3212,7 +3277,7 @@ static struct config_int ConfigureNamesInt[] =
},
&wal_retrieve_retry_interval,
5000, 1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3225,7 +3290,7 @@ static struct config_int ConfigureNamesInt[] =
DEFAULT_XLOG_SEG_SIZE,
WalSegMinSize,
WalSegMaxSize,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3236,7 +3301,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_naptime,
60, 1, INT_MAX / 1000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"autovacuum_vacuum_threshold", PGC_SIGHUP, AUTOVACUUM,
@@ -3245,7 +3310,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_vac_thresh,
50, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
{"autovacuum_vacuum_insert_threshold", PGC_SIGHUP, AUTOVACUUM,
@@ -3254,7 +3319,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_vac_ins_thresh,
1000, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled
},
{
{"autovacuum_analyze_threshold", PGC_SIGHUP, AUTOVACUUM,
@@ -3263,7 +3328,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_anl_thresh,
50, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
/* see varsup.c for why this is PGC_POSTMASTER not PGC_SIGHUP */
@@ -3278,7 +3343,7 @@ static struct config_int ConfigureNamesInt[] =
* upper-limit value.
*/
200000000, 100000, 2000000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
/* see multixact.c for why this is PGC_POSTMASTER not PGC_SIGHUP */
@@ -3288,7 +3353,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_multixact_freeze_max_age,
400000000, 10000, 2000000000,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
/* see max_connections */
@@ -3298,7 +3363,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_max_workers,
3, 1, MAX_BACKENDS,
- check_autovacuum_max_workers, NULL, NULL
+ check_autovacuum_max_workers, NULL, NULL, NULL
},
{
@@ -3308,7 +3373,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_parallel_maintenance_workers,
2, 0, 1024,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3319,7 +3384,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_parallel_workers_per_gather,
2, 0, MAX_PARALLEL_WORKER_LIMIT,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3330,7 +3395,7 @@ static struct config_int ConfigureNamesInt[] =
},
&max_parallel_workers,
8, 0, MAX_PARALLEL_WORKER_LIMIT,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3341,7 +3406,7 @@ static struct config_int ConfigureNamesInt[] =
},
&autovacuum_work_mem,
-1, -1, MAX_KILOBYTES,
- check_autovacuum_work_mem, NULL, NULL
+ check_autovacuum_work_mem, NULL, NULL, NULL
},
{
@@ -3352,7 +3417,7 @@ static struct config_int ConfigureNamesInt[] =
},
&old_snapshot_threshold,
-1, -1, MINS_PER_HOUR * HOURS_PER_DAY * 60,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled
},
{
@@ -3363,7 +3428,7 @@ static struct config_int ConfigureNamesInt[] =
},
&tcp_keepalives_idle,
0, 0, INT_MAX,
- NULL, assign_tcp_keepalives_idle, show_tcp_keepalives_idle
+ NULL, assign_tcp_keepalives_idle, show_tcp_keepalives_idle, special_default0
},
{
@@ -3374,7 +3439,7 @@ static struct config_int ConfigureNamesInt[] =
},
&tcp_keepalives_interval,
0, 0, INT_MAX,
- NULL, assign_tcp_keepalives_interval, show_tcp_keepalives_interval
+ NULL, assign_tcp_keepalives_interval, show_tcp_keepalives_interval, special_default0
},
{
@@ -3385,7 +3450,7 @@ static struct config_int ConfigureNamesInt[] =
},
&ssl_renegotiation_limit,
0, 0, 0,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3397,7 +3462,7 @@ static struct config_int ConfigureNamesInt[] =
},
&tcp_keepalives_count,
0, 0, INT_MAX,
- NULL, assign_tcp_keepalives_count, show_tcp_keepalives_count
+ NULL, assign_tcp_keepalives_count, show_tcp_keepalives_count, special_default0
},
{
@@ -3408,7 +3473,7 @@ static struct config_int ConfigureNamesInt[] =
},
&GinFuzzySearchLimit,
0, 0, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3420,7 +3485,7 @@ static struct config_int ConfigureNamesInt[] =
},
&effective_cache_size,
DEFAULT_EFFECTIVE_CACHE_SIZE, 1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3431,7 +3496,7 @@ static struct config_int ConfigureNamesInt[] =
},
&min_parallel_table_scan_size,
(8 * 1024 * 1024) / BLCKSZ, 0, INT_MAX / 3,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3442,7 +3507,7 @@ static struct config_int ConfigureNamesInt[] =
},
&min_parallel_index_scan_size,
(512 * 1024) / BLCKSZ, 0, INT_MAX / 3,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3454,7 +3519,7 @@ static struct config_int ConfigureNamesInt[] =
},
&server_version_num,
PG_VERSION_NUM, PG_VERSION_NUM, PG_VERSION_NUM,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3465,7 +3530,7 @@ static struct config_int ConfigureNamesInt[] =
},
&log_temp_files,
-1, -1, INT_MAX,
- NULL, NULL, NULL
+ NULL, NULL, NULL, special_disabled_all
},
{
@@ -3476,7 +3541,7 @@ static struct config_int ConfigureNamesInt[] =
},
&pgstat_track_activity_query_size,
1024, 100, 1048576,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3487,7 +3552,7 @@ static struct config_int ConfigureNamesInt[] =
},
&gin_pending_list_limit,
4096, 64, MAX_KILOBYTES,
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3498,7 +3563,7 @@ static struct config_int ConfigureNamesInt[] =
},
&tcp_user_timeout,
0, 0, INT_MAX,
- NULL, assign_tcp_user_timeout, show_tcp_user_timeout
+ NULL, assign_tcp_user_timeout, show_tcp_user_timeout, special_default0
},
{
@@ -3532,7 +3597,7 @@ static struct config_int ConfigureNamesInt[] =
#else /* not DISCARD_CACHES_ENABLED */
0, 0, 0,
#endif /* not DISCARD_CACHES_ENABLED */
- NULL, NULL, NULL
+ NULL, NULL, NULL, NULL
},
{
@@ -3543,12 +3608,12 @@ static struct config_int ConfigureNamesInt[] =
},
&client_connection_check_interval,
0, 0, INT_MAX,
- check_client_connection_check_interval, NULL, NULL
+ check_client_connection_check_interval, NULL, NULL, special_disabled0
},
/* End-of-list marker */
{
- {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL
+ {NULL, 0, 0, NULL, NULL}, NULL, 0, 0, 0, NULL, NULL, NULL, NULL
}
};
@@ -6911,6 +6976,56 @@ parse_int(const char *value, int *result, int flags, const char **hintmsg)
return true;
}
+
+/*
+ * Lookup special values from an array of chars -> int (case-sensitive).
+ * If the value is found, sets the retval value and returns
+ * true. If it's not found, return false and retval is set to 0.
+ */
+bool
+parse_special_int(const struct config_enum_entry *options, const char *value,
+ int *retval)
+{
+ const struct config_enum_entry *entry;
+
+ for (entry = options; entry && entry->name; entry++)
+ {
+ if (pg_strcasecmp(value, entry->name) == 0)
+ {
+ *retval = entry->val;
+ return true;
+ }
+ }
+
+ /* don't touch the return value in other case */
+ return false;
+}
+
+/*
+ * Lookup special values from an array of chars -> int (case-sensitive).
+ * If the value is found, sets the retval value and returns
+ * true. If it's not found, return false and retval is set to 0.
+ */
+bool
+special_int_to_value(const struct config_enum_entry *options, int value,
+ const char **retval)
+{
+ const struct config_enum_entry *entry;
+
+ for (entry = options; entry && entry->name; entry++)
+ {
+ if (value == entry->val)
+ {
+ *retval = entry->name;
+ return true;
+ }
+ }
+
+ /* don't touch the return value in other case */
+ return false;
+}
+
+
/*
* Try to parse value as a floating point number in the usual format.
* Optionally, the value can be followed by a unit name if "flags" indicates
@@ -6974,7 +7089,6 @@ parse_real(const char *value, double *result, int flags, const char **hintmsg)
return true;
}
-
/*
* Lookup the name for an enum option with the selected value.
* Should only ever be called with known-valid values, so throws
@@ -7121,8 +7235,8 @@ parse_and_validate_value(struct config_generic *record,
struct config_int *conf = (struct config_int *) record;
const char *hintmsg;
- if (!parse_int(value, &newval->intval,
- conf->gen.flags, &hintmsg))
+ if (!(conf->special && parse_special_int(conf->special, value, &newval->intval)) &&
+ !parse_int(value, &newval->intval, conf->gen.flags, &hintmsg))
{
ereport(elevel,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
@@ -10123,7 +10237,11 @@ _ShowOption(struct config_generic *record, bool use_units)
{
struct config_int *conf = (struct config_int *) record;
- if (conf->show_hook)
+ /* Special values are prioritized over show hooks */
+ if (output_special_values && conf->special && special_int_to_value(conf->special, *conf->variable,
&val))
+ /* if return is true we have no special action to take here but val was set already */
+ ;
+ else if (conf->show_hook)
val = conf->show_hook();
else
{
diff --git a/src/include/utils/guc_tables.h b/src/include/utils/guc_tables.h
index 6b40f1eeb8..ef2b5aa4c2 100644
--- a/src/include/utils/guc_tables.h
+++ b/src/include/utils/guc_tables.h
@@ -197,6 +197,7 @@ struct config_int
GucIntCheckHook check_hook;
GucIntAssignHook assign_hook;
GucShowHook show_hook;
+ const struct config_enum_entry *special;
/* variable fields, initialized at runtime: */
int reset_val;
void *reset_extra;
diff --git a/src/test/regress/expected/guc.out b/src/test/regress/expected/guc.out
index 59da91ff04..af12c4d1c3 100644
--- a/src/test/regress/expected/guc.out
+++ b/src/test/regress/expected/guc.out
@@ -813,3 +813,48 @@ set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
ERROR: tables declared WITH OIDS are not supported
+-- tests for output_special_values and special values
+set output_special_values to t;
+set log_min_duration_statement = 100;
+show log_min_duration_statement;
+ log_min_duration_statement
+----------------------------
+ 100ms
+(1 row)
+
+set log_min_duration_statement = -1;
+show log_min_duration_statement;
+ log_min_duration_statement
+----------------------------
+ disabled
+(1 row)
+
+set log_min_duration_statement = disabled;
+show log_min_duration_statement;
+ log_min_duration_statement
+----------------------------
+ disabled
+(1 row)
+
+set output_special_values to f;
+set log_min_duration_statement = 100;
+show log_min_duration_statement;
+ log_min_duration_statement
+----------------------------
+ 100ms
+(1 row)
+
+set log_min_duration_statement = -1;
+show log_min_duration_statement;
+ log_min_duration_statement
+----------------------------
+ -1
+(1 row)
+
+set log_min_duration_statement = disabled;
+show log_min_duration_statement;
+ log_min_duration_statement
+----------------------------
+ -1
+(1 row)
+
diff --git a/src/test/regress/sql/guc.sql b/src/test/regress/sql/guc.sql
index c39c11388d..b6a54e0290 100644
--- a/src/test/regress/sql/guc.sql
+++ b/src/test/regress/sql/guc.sql
@@ -311,3 +311,19 @@ reset check_function_bodies;
set default_with_oids to f;
-- Should not allow to set it to true.
set default_with_oids to t;
+
+-- tests for output_special_values and special values
+set output_special_values to t;
+set log_min_duration_statement = 100;
+show log_min_duration_statement;
+set log_min_duration_statement = -1;
+show log_min_duration_statement;
+set log_min_duration_statement = disabled;
+show log_min_duration_statement;
+set output_special_values to f;
+set log_min_duration_statement = 100;
+show log_min_duration_statement;
+set log_min_duration_statement = -1;
+show log_min_duration_statement;
+set log_min_duration_statement = disabled;
+show log_min_duration_statement;
--
2.30.1 (Apple Git-130)
--
On 8/20/21 12:09 AM, David Christensen wrote: > -hackers, > > Enclosed, find a POC patch that implements "special values" for int GUCs. We have quite a few GUCs > with values that have special meaning atop other settings. I have attempted to identify these and > make it so you can specify a symbol name for these values instead of just relying on the magic > number instead. > > For instance, many GUCs have -1 for "disabled", so I've just made it so you can > specify something like: > > SET log_min_duration_statement = disabled; > > And the raw value will be set to -1 in this case. For the purposes of testing, I have also added a > new GUC "output_special_values" to affect whether `SHOW` or anything that relies on _ShowOption() > can show with the special value instead of just the raw magic value, allowing tools to consume the > original raw value, or provide the output to the user in the nicer format. > > This has only been done for ints, and the passthru I did was very quick, so I have probably missed > some options that didn't explicitly have their interpretations in the file and/or I didn't know > about it already. I do not think there are these sorts of values in other non-int GUCs, but there > might be, so a similar approach could be taken to expand things to other config types in the future. > > Let me know your thoughts; I personally find this to be useful, and would be a nicer way for some > configs to be displayed in the postgresql.conf file. As discussed on IRC, I am in favor of this improvement. (I have not yet looked at the patch.) -- Vik Fearing
> Hi, > For parse_special_int(): > > + * true. If it's not found, return false and retval is set to 0. > ... > + /* don't touch the return value in other case */ > + return false; > > It seems the two comments are not consistent with each other (retval is not set in case no entry is found). > > For special_int_to_value(): > > + * true. If it's not found, return false and retval is set to 0. > > First, there is no assignment to retval at the end of the method. Second, retval points to string, so it shouldn't be setto 0. > > Cheers Thanks, I actually noticed on a re-read that the comments didn't match, but they'll be fixed in the next version. (Will wait to collect additional feedback.) Functionality-wise, any thoughts on the overall approach or the specific patch? Thanks, David
On Thu, Aug 19, 2021 at 3:17 PM David Christensen <david.christensen@crunchydata.com> wrote:
-hackers,
Enclosed, find a POC patch that implements "special values" for int GUCs. We have quite a few GUCs
with values that have special meaning atop other settings. I have attempted to identify these and
make it so you can specify a symbol name for these values instead of just relying on the magic
number instead.
For instance, many GUCs have -1 for "disabled", so I've just made it so you can
specify something like:
SET log_min_duration_statement = disabled;
And the raw value will be set to -1 in this case. For the purposes of testing, I have also added a
new GUC "output_special_values" to affect whether `SHOW` or anything that relies on _ShowOption()
can show with the special value instead of just the raw magic value, allowing tools to consume the
original raw value, or provide the output to the user in the nicer format.
This has only been done for ints, and the passthru I did was very quick, so I have probably missed
some options that didn't explicitly have their interpretations in the file and/or I didn't know
about it already. I do not think there are these sorts of values in other non-int GUCs, but there
might be, so a similar approach could be taken to expand things to other config types in the future.
Let me know your thoughts; I personally find this to be useful, and would be a nicer way for some
configs to be displayed in the postgresql.conf file.
Best,
David
--
Hi,
For parse_special_int():
+ * true. If it's not found, return false and retval is set to 0.
...
+ /* don't touch the return value in other case */
+ return false;
+ return false;
It seems the two comments are not consistent with each other (retval is not set in case no entry is found).
For special_int_to_value():
+ * true. If it's not found, return false and retval is set to 0.
First, there is no assignment to retval at the end of the method. Second, retval points to string, so it shouldn't be set to 0.
Cheers
On Thu, Aug 19, 2021 at 3:44 PM David Christensen <david.christensen@crunchydata.com> wrote:
Functionality-wise, any thoughts on the overall approach or the specific patch?
If this information was exposed only by an addition to pg_settings, and thus not changeable via a GUC or affecting SHOW, I think it would be more likely to get accepted. Being more liberal in the accepting of these values as input seems less problematic so that aspect could just stay. But the display changes should be done with new outputs, not repurposing existing ones.
I'm at -0.5 as to whether such a patch would actually be an improvement or whether the added possibilities would just be confusing and, because it is all optional, indefinitely so.
David J.
On Thu, Aug 19, 2021 at 03:58:57PM -0700, David G. Johnston wrote: > I'm at -0.5 as to whether such a patch would actually be an improvement or > whether the added possibilities would just be confusing and, because it is > all optional, indefinitely so. FWIW, I find this proposition of introducing a set of optional synonyms to map with some special-case values we have in the configurations a bit confusing, as that's basically introducing enum-like options into GUCs that already have a type assigned. The patch, with its set of options like special_disabled0, special_disabled_all is not really easy to parse either so that's just a recipe to make the set of synonyms to grow on an GUC-basis. What I am wondering, though, is if there are cases in the existing GUCs, with their existing types, where the situation of a default or disabled value could be improved, though, to make the overall picture more consistent. -- Michael
Вложения
On Fri, Aug 27, 2021 at 1:19 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Aug 19, 2021 at 03:58:57PM -0700, David G. Johnston wrote: > > I'm at -0.5 as to whether such a patch would actually be an improvement or > > whether the added possibilities would just be confusing and, because it is > > all optional, indefinitely so. > > FWIW, I find this proposition of introducing a set of optional > synonyms to map with some special-case values we have in the > configurations a bit confusing, as that's basically introducing > enum-like options into GUCs that already have a type assigned. It does use enum-like mappings, but that is just because I needed to tie together name + value and just reused the already similar data structure. That could be changed if the code itself is less understandable based on the struct names. > The patch, with its set of options like special_disabled0, > special_disabled_all is not really easy to parse either so that's just > a recipe to make the set of synonyms to grow on an GUC-basis. Yes, when I started out on this, I expected maybe 2-3 different interpretations at most, with more common overlap. I am not tied to making *every* GUC support this; maybe we support the special_disabled or special_disabled0 with differing names. > What I am wondering, though, is if there are cases in the existing > GUCs, with their existing types, where the situation of a default or > disabled value could be improved, though, to make the overall picture > more consistent. I think this would be possible, although the benefit of what I've written is that it doesn't change the interpretation of the value anywhere else, just in GUC parsing (and optionally GUC display). The parsing was where I felt this improved understanding, I'm less tied to outputting in the "canonical" way.
It looks like this patch rotted a little and needs to be rebased. Please see http://cfbot.cputube.org/ The new status of this patch is: Waiting on Author
Updated version attached with comment fixes and updated for new GUC.
Вложения
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: tested, passed Documentation: not tested Hi I quite like the feature this patch provides, it makes special int values more meaningful to user. I like that you addeda configuration to allow the original int value to be outputted or the special value, this adds some flexibility incase returning special value breaks some old applications. I scanned through the GUC list and found that the followingparameters can potentially be categorized in the "special_disabled0" group, just for your reference. max_prepared_transactions vacuum_cost_delay effective_io_concurrency max_parallel_workers max_parallel_maintenance_workers max_parallel_workers_per_gather max_logical_replication_workers max_sync_workers_per_subscription jit_above_cost jit_inline_above_cost jit_optimize_above_cost log_rotation_age log_rotation_size log_transaction_sample_rate Cary Huang ----------------- HighGo Software Canada www.highgo.ca
> On 15 Oct 2021, at 23:54, Cary Huang <cary.huang@highgo.ca> wrote: > I scanned through the GUC list and found that the following parameters can > potentially be categorized in the "special_disabled0" group, just for your > reference. This patch no longer applies, can you please submit a rebased version? Also, do you have any thoughts on Cary's suggestion in the above review? -- Daniel Gustafsson https://vmware.com/
> On Nov 3, 2021, at 5:35 AM, Daniel Gustafsson <daniel@yesql.se> wrote: > > >> >>> On 15 Oct 2021, at 23:54, Cary Huang <cary.huang@highgo.ca> wrote: >> >> I scanned through the GUC list and found that the following parameters can >> potentially be categorized in the "special_disabled0" group, just for your >> reference. > > > This patch no longer applies, can you please submit a rebased version? Also, > do you have any thoughts on Cary's suggestion in the above review? Hi, enclosed is a v3, which includes the rebase as well as the additional int GUCs mentioned in Cary’s review. I can addsupport for Reals (required for the JIT and several other ones), but wanted to get feedback/buy-in on the overall ideafirst. Best, David
Вложения
Hi, On Thu, Nov 4, 2021 at 5:50 AM David Christensen <david@pgguru.net> wrote: > > Hi, enclosed is a v3 [...] According to the cfbot, the patch doesn't apply anymore and needs a rebase: http://cfbot.cputube.org/patch_36_3290.log > 43 out of 133 hunks FAILED -- saving rejects to file src/backend/utils/misc/guc.c.rej I'm switching the patch to Waiting on Author.
> Hi, > > According to the cfbot, the patch doesn't apply anymore and needs a > rebase: http://cfbot.cputube.org/patch_36_3290.log V4 rebased attached.
Вложения
Hi, On 2022-01-12 12:57:02 -0600, David Christensen wrote: > > Hi, > > > > According to the cfbot, the patch doesn't apply anymore and needs a > > rebase: http://cfbot.cputube.org/patch_36_3290.log > > V4 rebased attached. Doesn't apply anymore, again: http://cfbot.cputube.org/patch_37_3290.log My impression is that there's not a lot of enthusiasm for the concept? If that's true we maybe ought to mark the CF entry as rejected? Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> My impression is that there's not a lot of enthusiasm for the concept? If
> that's true we maybe ought to mark the CF entry as rejected?
Yeah, I'm kind of leaning that way too. I don't see how we can
incorporate the symbolic values into any existing display paths
without breaking applications that expect the old output.
That being the case, it seems like we'd have "two ways to do it"
indefinitely, which would add enough confusion that I'm not
sure there's a net gain. In particular, I foresee novice questions
along the lines of "I set foo to disabled, why is it showing
as zero?".
If we'd done it like this from the beginning, it'd have been
great, but retrofitting it now is a lot less appealing.
regards, tom lane
> On Mar 21, 2022, at 7:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Andres Freund <andres@anarazel.de> writes: >> My impression is that there's not a lot of enthusiasm for the concept? If >> that's true we maybe ought to mark the CF entry as rejected? > > Yeah, I'm kind of leaning that way too. I don't see how we can > incorporate the symbolic values into any existing display paths > without breaking applications that expect the old output. > That being the case, it seems like we'd have "two ways to do it" > indefinitely, which would add enough confusion that I'm not > sure there's a net gain. In particular, I foresee novice questions > along the lines of "I set foo to disabled, why is it showing > as zero?" Yeah, my main motivation here was about trying to have less special values in the config files, but I guess it would effectivelybe making everything effectively an enum and still relies on knowing just what the specific magic values are,no not really a net gain in this department. For the record, I thought this would have a fairly low chance of getting in, was mainly curious what level of effort it wouldtake to get something like this going and get some feedback on the actual idea. > If we'd done it like this from the beginning, it'd have been > great, but retrofitting it now is a lot less appealing. Yeah, agreed on this. As far as I’m concerned we can reject. Thanks, David
On 2022-03-22 13:15:34 -0500, David Christensen wrote: > > On Mar 21, 2022, at 7:53 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > If we'd done it like this from the beginning, it'd have been > > great, but retrofitting it now is a lot less appealing. > > Yeah, agreed on this. As far as I’m concerned we can reject. Updated CF entry to say so...