From cabc9e35cb17fecbb7680bf0c5c7d4f2c4478432 Mon Sep 17 00:00:00 2001 From: Robert Haas Date: Mon, 30 Mar 2026 19:10:32 -0400 Subject: [PATCH v2 2/3] Add a guc_check_handler to the EXPLAIN extension mechanism. It would be useful to be able to tell auto_explain to set a custom EXPLAIN option, but it would be bad if it tried to do so and the option name or value wasn't valid, because then every query would fail with a complaint about the EXPLAIN option. So add a guc_check_handler that auto_explain will be able to use to only try to set option name/value/type combinations that have been determined to be legal, and to emit useful messages about ones that aren't. Reviewed-by: Matheus Alcantara Discussion: http://postgr.es/m/CA+Tgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w@mail.gmail.com --- contrib/pg_overexplain/pg_overexplain.c | 6 +- contrib/pg_plan_advice/pg_plan_advice.c | 3 +- src/backend/commands/explain_state.c | 121 +++++++++++++++++++++++- src/include/commands/explain_state.h | 13 ++- 4 files changed, 135 insertions(+), 8 deletions(-) diff --git a/contrib/pg_overexplain/pg_overexplain.c b/contrib/pg_overexplain/pg_overexplain.c index b4e90909289..715eda8dc56 100644 --- a/contrib/pg_overexplain/pg_overexplain.c +++ b/contrib/pg_overexplain/pg_overexplain.c @@ -73,9 +73,11 @@ _PG_init(void) es_extension_id = GetExplainExtensionId("pg_overexplain"); /* Register the new EXPLAIN options implemented by this module. */ - RegisterExtensionExplainOption("debug", overexplain_debug_handler); + RegisterExtensionExplainOption("debug", overexplain_debug_handler, + GUCCheckBooleanExplainOption); RegisterExtensionExplainOption("range_table", - overexplain_range_table_handler); + overexplain_range_table_handler, + GUCCheckBooleanExplainOption); /* Use the per-node and per-plan hooks to make our options do something. */ prev_explain_per_node_hook = explain_per_node_hook; diff --git a/contrib/pg_plan_advice/pg_plan_advice.c b/contrib/pg_plan_advice/pg_plan_advice.c index 2393ed55518..5629070faf4 100644 --- a/contrib/pg_plan_advice/pg_plan_advice.c +++ b/contrib/pg_plan_advice/pg_plan_advice.c @@ -126,7 +126,8 @@ _PG_init(void) /* Register the new EXPLAIN options implemented by this module. */ RegisterExtensionExplainOption("plan_advice", - pg_plan_advice_explain_option_handler); + pg_plan_advice_explain_option_handler, + GUCCheckBooleanExplainOption); /* Install hooks */ pgpa_planner_install_hooks(); diff --git a/src/backend/commands/explain_state.c b/src/backend/commands/explain_state.c index 77f59b8e500..65dd4111459 100644 --- a/src/backend/commands/explain_state.c +++ b/src/backend/commands/explain_state.c @@ -36,6 +36,8 @@ #include "commands/defrem.h" #include "commands/explain.h" #include "commands/explain_state.h" +#include "utils/builtins.h" +#include "utils/guc.h" /* Hook to perform additional EXPLAIN options validation */ explain_validate_options_hook_type explain_validate_options_hook = NULL; @@ -44,6 +46,7 @@ typedef struct { const char *option_name; ExplainOptionHandler option_handler; + ExplainOptionGUCCheckHandler guc_check_handler; } ExplainExtensionOption; static const char **ExplainExtensionNameArray = NULL; @@ -304,26 +307,39 @@ SetExplainExtensionState(ExplainState *es, int extension_id, void *opaque) /* * Register a new EXPLAIN option. * + * option_name is assumed to be a constant string or allocated in storage + * that will never be freed. + * * When option_name is used as an EXPLAIN option, handler will be called and * should update the ExplainState passed to it. See comments at top of file * for a more detailed explanation. * - * option_name is assumed to be a constant string or allocated in storage - * that will never be freed. + * guc_check_handler is a function that can be safely called from a + * GUC check hook to validate a proposed value for a custom EXPLAIN option. + * Boolean-valued options can pass GUCCheckBooleanExplainOption. See the + * comments for GUCCheckBooleanExplainOption for further information on + * how a guc_check_handler should behave. */ void RegisterExtensionExplainOption(const char *option_name, - ExplainOptionHandler handler) + ExplainOptionHandler handler, + ExplainOptionGUCCheckHandler guc_check_handler) { ExplainExtensionOption *exopt; + Assert(handler != NULL); + Assert(guc_check_handler != NULL); + /* Search for an existing option by this name; if found, update handler. */ for (int i = 0; i < ExplainExtensionOptionsAssigned; ++i) { if (strcmp(ExplainExtensionOptionArray[i].option_name, option_name) == 0) { - ExplainExtensionOptionArray[i].option_handler = handler; + exopt = &ExplainExtensionOptionArray[i]; + + exopt->option_handler = handler; + exopt->guc_check_handler = guc_check_handler; return; } } @@ -352,6 +368,7 @@ RegisterExtensionExplainOption(const char *option_name, exopt = &ExplainExtensionOptionArray[ExplainExtensionOptionsAssigned++]; exopt->option_name = option_name; exopt->option_handler = handler; + exopt->guc_check_handler = guc_check_handler; } /* @@ -375,3 +392,99 @@ ApplyExtensionExplainOption(ExplainState *es, DefElem *opt, ParseState *pstate) return false; } + +/* + * Determine whether an EXPLAIN extension option will be accepted without + * error. Returns true if so, and false if not. See the comments for + * GUCCheckBooleanExplainOption for more details. + * + * The caller need not know that the option_name is valid; this function + * will indicate that the option is unrecognized if that is the case. + */ +bool +GUCCheckExplainExtensionOption(const char *option_name, + const char *option_value, + NodeTag option_type) +{ + for (int i = 0; i < ExplainExtensionOptionsAssigned; i++) + { + ExplainExtensionOption *exopt = &ExplainExtensionOptionArray[i]; + + if (strcmp(exopt->option_name, option_name) == 0) + return exopt->guc_check_handler(option_name, option_value, + option_type); + } + + /* Unrecognized option name. */ + GUC_check_errmsg("unrecognized EXPLAIN option \"%s\"", option_name); + return false; +} + +/* + * guc_check_handler for Boolean-valued EXPLAIN extension options. + * + * After receiving a "true" value from this or any other GUC check handler + * for an EXPLAIN extension option, the caller is entitled to assume that + * a suitably constructed DefElem passed to the main option handler will + * not cause an error. To construct this DefElem, the caller should set + * the DefElem's defname to option_name. If option_values is NULL, arg + * should be NULL. Otherwise, arg should be of the type given by + * option_type, with option_value as the associated value. The only option + * types that should be passed are T_String, T_Float, and T_Integer; in + * the last case, the caller will need to perform a string-to-integer + * conversion. + * + * A guc_check_handler should not throw an error, and should not allocate + * memory. If it returns false to indicate that the option_value is not + * acceptable, it may use GUC_check_errmsg(), GUC_check_errdetail(), etc. + * to clarify the nature of the problem. + * + * Since we're concerned with Boolean options here, the logic below must + * exactly match the semantics of defGetBoolean. + */ +bool +GUCCheckBooleanExplainOption(const char *option_name, + const char *option_value, + NodeTag option_type) +{ + bool valid = false; + + if (option_value == NULL) + { + /* defGetBoolean treats no argument as valid */ + valid = true; + } + else if (option_type == T_String) + { + /* defGetBoolean accepts exactly these string values */ + if (pg_strcasecmp(option_value, "true") == 0 || + pg_strcasecmp(option_value, "false") == 0 || + pg_strcasecmp(option_value, "on") == 0 || + pg_strcasecmp(option_value, "off") == 0) + valid = true; + } + else if (option_type == T_Integer) + { + long value; + char *end; + + /* + * defGetBoolean accepts only 0 and 1, but those can be spelled in + * various ways (e.g. 01, 0x01). + */ + errno = 0; + value = strtol(option_value, &end, 0); + if (errno == 0 && *end == '\0' && end != option_value && + value == (int) value && (value == 0 || value == 1)) + valid = true; + } + + if (!valid) + { + GUC_check_errmsg("EXPLAIN option \"%s\" requires a Boolean value", + option_name); + return false; + } + + return true; +} diff --git a/src/include/commands/explain_state.h b/src/include/commands/explain_state.h index 5a48bc6fbb1..6252fe11f15 100644 --- a/src/include/commands/explain_state.h +++ b/src/include/commands/explain_state.h @@ -78,6 +78,9 @@ typedef struct ExplainState } ExplainState; typedef void (*ExplainOptionHandler) (ExplainState *, DefElem *, ParseState *); +typedef bool (*ExplainOptionGUCCheckHandler) (const char *option_name, + const char *option_value, + NodeTag option_type); /* Hook to perform additional EXPLAIN options validation */ typedef void (*explain_validate_options_hook_type) (ExplainState *es, List *options, @@ -94,8 +97,16 @@ extern void SetExplainExtensionState(ExplainState *es, int extension_id, void *opaque); extern void RegisterExtensionExplainOption(const char *option_name, - ExplainOptionHandler handler); + ExplainOptionHandler handler, + ExplainOptionGUCCheckHandler guc_check_handler); extern bool ApplyExtensionExplainOption(ExplainState *es, DefElem *opt, ParseState *pstate); +extern bool GUCCheckExplainExtensionOption(const char *option_name, + const char *option_value, + NodeTag option_type); + +extern bool GUCCheckBooleanExplainOption(const char *option_name, + const char *option_value, + NodeTag option_type); #endif /* EXPLAIN_STATE_H */ -- 2.51.0