public inbox for [email protected]help / color / mirror / Atom feed
Add custom EXPLAIN options support to auto_explain 16+ messages / 3 participants [nested] [flat]
* Add custom EXPLAIN options support to auto_explain @ 2026-03-26 19:18 Matheus Alcantara <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Matheus Alcantara @ 2026-03-26 19:18 UTC (permalink / raw) To: [email protected] Hi, Attached patch add a new GUC parameter auto_explain.log_options that accepts a comma-separated list of custom EXPLAIN options registered by extensions. This allows auto_explain to pass extension specific options (like from pg_plan_advice) when logging query plans. Based on pg_plan_advice and pg_overexplain use of custom explain options I see two different cases: 1. pg_plan_advice check at planner_setup_hook() if custom explain option is enabled or not to decide if the advice should be collected. 2. Since pg_overexplain don't collect any other data (just print more planner information) it only check at explain_per_plan_hook() if the custom explain option is enabled or not. So it seems to me that we have two patterns here: 1. Custom extensions that want to include more information during planning so in this case it should use the planner_setup_hook() and 2. which are extensions that don't need any extra planner information and can just hook explain_per_plan_hook(). That being said, this patch creates a new planner_setup_hook for case 1 and changes explain_ExecutorEnd() to call explain_per_plan_hook() for case 2. Note that even for case 1, we still need to call explain_per_plan_hook() so the extra information from the custom explain option is included in the explain output. Regarding the explain_per_plan_hook() call in explain_ExecutorEnd(): normally this hook is called by ExplainOnePlan() during a regular EXPLAIN command. However, auto_explain doesn't go through ExplainOnePlan() - it builds its own ExplainState and calls the individual explain functions (ExplainPrintPlan, ExplainPrintTriggers, ExplainPrintJITSummary) directly. We can't use ExplainOnePlan() because it expects to execute the query itself, whereas auto_explain runs after execution is already complete (inside the ExecutorEnd hook) and already has a QueryDesc with execution results. Since there's no existing helper function that handles just the "output explain for an already-executed query" portion while also calling explain_per_plan_hook(), the only option currently is to call explain_per_plan_hook() directly. I'm wondering if we should create such a helper function, or if there's a better approach here? -- Matheus Alcantara EDB: https://www.enterprisedb.com From d493f789d50637574d186802ebe5937e2659341a Mon Sep 17 00:00:00 2001 From: Matheus Alcantara <[email protected]> Date: Thu, 26 Mar 2026 12:54:15 -0300 Subject: [PATCH v1] Add custom EXPLAIN options support to auto_explain Add a new GUC parameter auto_explain.log_options that accepts a comma-separated list of custom EXPLAIN options registered by extensions. This allows auto_explain to pass extension specific options (like those from pg_plan_advice) when logging query plans. Extensions that register custom EXPLAIN options follow two patterns: 1. Some extensions (e.g., pg_plan_advice) check during planning whether their custom option is enabled, to decide if extra information should be collected. To support this, a new planner_setup_hook is added that creates an ExplainState with the configured options. 2. Other extensions (e.g., pg_overexplain) only check at output time whether their option is enabled, using explain_per_plan_hook. Normally, explain_per_plan_hook is called by ExplainOnePlan() during a regular EXPLAIN command. However, auto_explain generates explain output manually at ExecutorEnd by calling ExplainPrintPlan and related functions directly, since ExplainOnePlan expects to execute the query itself. Therefore, this commit adds an explicit call to explain_per_plan_hook in explain_ExecutorEnd to allow extensions to include their output. --- contrib/auto_explain/auto_explain.c | 157 ++++++++++++++++++++++++++++ doc/src/sgml/auto-explain.sgml | 19 ++++ 2 files changed, 176 insertions(+) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index e856cd35a6f..ff794394306 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -20,7 +20,11 @@ #include "commands/explain_state.h" #include "common/pg_prng.h" #include "executor/instrument.h" +#include "nodes/makefuncs.h" +#include "optimizer/planner.h" +#include "tcop/tcopprot.h" #include "utils/guc.h" +#include "utils/varlena.h" PG_MODULE_MAGIC_EXT( .name = "auto_explain", @@ -41,6 +45,7 @@ static int auto_explain_log_format = EXPLAIN_FORMAT_TEXT; static int auto_explain_log_level = LOG; static bool auto_explain_log_nested_statements = false; static double auto_explain_sample_rate = 1; +static char *auto_explain_log_options = NULL; static const struct config_enum_entry format_options[] = { {"text", EXPLAIN_FORMAT_TEXT, false}, @@ -76,11 +81,15 @@ static bool current_query_sampled = false; current_query_sampled) /* Saved hook values */ +static planner_setup_hook_type prev_planner_setup = NULL; static ExecutorStart_hook_type prev_ExecutorStart = NULL; static ExecutorRun_hook_type prev_ExecutorRun = NULL; static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; +static void explain_planner_setup(PlannerGlobal *glob, Query *parse, + const char *query_string, int cursorOptions, + double *tuple_fraction, ExplainState *es); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, @@ -88,6 +97,125 @@ static void explain_ExecutorRun(QueryDesc *queryDesc, static void explain_ExecutorFinish(QueryDesc *queryDesc); static void explain_ExecutorEnd(QueryDesc *queryDesc); +/* + * Apply custom EXPLAIN options from auto_explain.log_options to the + * ExplainState. + * + * This parses the comma-separated option list and calls + * ApplyExtensionExplainOption for each one. + */ +static void +apply_custom_options(ExplainState *es) +{ + char *options; + List *elemlist; + ListCell *lc; + + if (auto_explain_log_options == NULL || auto_explain_log_options[0] == '\0') + return; + + options = pstrdup(auto_explain_log_options); + + if (!SplitIdentifierString(options, ',', &elemlist)) + { + /* Shouldn't happen since check_explain_options validated this */ + Assert(false); + pfree(options); + return; + } + + foreach(lc, elemlist) + { + const char *option = (const char *) lfirst(lc); + DefElem *def; + + /* + * Create a DefElem for this option. Pass NULL as the argument, which + * for boolean options means "true". + */ + def = makeDefElem(pstrdup(option), NULL, -1); + + /* + * Apply the option. If the extension that registered this option is + * not loaded, ApplyExtensionExplainOption will return false, which we + * silently ignore. This allows the GUC to be set even if the + * extension providing the option isn't currently loaded. + * + * XXX: ParseState is not used by pg_plan_advice and pg_overexplain. + * Using NULL is a problem for future extensions? + */ + ApplyExtensionExplainOption(es, def, NULL); + } + + pfree(options); + list_free(elemlist); +} + +/* GUC check hook for auto_explain.log_options. */ +static bool +check_explain_options(char **newval, void **extra, GucSource source) +{ + char *options; + List *elemlist; + + if (*newval == NULL || (*newval)[0] == '\0') + return true; + + /* Make a modifiable copy */ + options = pstrdup(*newval); + + if (!SplitIdentifierString(options, ',', &elemlist)) + { + GUC_check_errdetail("Invalid syntax in option list."); + pfree(options); + list_free(elemlist); + return false; + } + + pfree(options); + list_free(elemlist); + return true; +} + +/* + * Planner setup hook: pass custom EXPLAIN options to planner extensions. + * + * Extensions like pg_plan_advice need to know about custom EXPLAIN options + * during planning so they can generate data that will be displayed later. + * When auto_explain.log_options is configured and auto_explain is potentially + * active, we create an ExplainState with those options and pass it to the + * planner hook chain. + */ +static void +explain_planner_setup(PlannerGlobal *glob, Query *parse, + const char *query_string, int cursorOptions, + double *tuple_fraction, ExplainState *es) +{ + /* + * If auto_explain is potentially active (log_min_duration >= 0) and we + * have custom options configured, create an ExplainState with those + * options applied. This signals to extensions like pg_plan_advice that + * they should generate data for these options. + * + * We do this even if the caller already provided an ExplainState (i.e., + * we're inside an EXPLAIN command), because our options might differ. + * However, if an ExplainState is already provided, extensions will see + * that one first, so we only create ours if es is NULL. + */ + if (auto_explain_log_min_duration >= 0 && + auto_explain_log_options != NULL && + auto_explain_log_options[0] != '\0') + { + if (es == NULL) + es = NewExplainState(); + + apply_custom_options(es); + } + + if (prev_planner_setup) + prev_planner_setup(glob, parse, query_string, cursorOptions, + tuple_fraction, es); +} /* * Module load callback @@ -245,9 +373,22 @@ _PG_init(void) NULL, NULL); + DefineCustomStringVariable("auto_explain.log_options", + "Custom EXPLAIN options to include in plan logging.", + "Comma-separated list of custom EXPLAIN options registered by extensions. ", + &auto_explain_log_options, + NULL, + PGC_SUSET, + GUC_LIST_INPUT, + check_explain_options, + NULL, + NULL); + MarkGUCPrefixReserved("auto_explain"); /* Install hooks. */ + prev_planner_setup = planner_setup_hook; + planner_setup_hook = explain_planner_setup; prev_ExecutorStart = ExecutorStart_hook; ExecutorStart_hook = explain_ExecutorStart; prev_ExecutorRun = ExecutorRun_hook; @@ -404,6 +545,9 @@ explain_ExecutorEnd(QueryDesc *queryDesc) es->format = auto_explain_log_format; es->settings = auto_explain_log_settings; + /* Apply any custom EXPLAIN options */ + apply_custom_options(es); + ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); ExplainQueryParameters(es, queryDesc->params, auto_explain_log_parameter_max_length); @@ -412,6 +556,19 @@ explain_ExecutorEnd(QueryDesc *queryDesc) ExplainPrintTriggers(es, queryDesc); if (es->costs) ExplainPrintJITSummary(es, queryDesc); + + /* + * Allow plugins to print additional EXPLAIN information. This + * mirrors what ExplainOnePlan does, allowing extensions that use + * explain_per_plan_hook to add their output. + */ + if (explain_per_plan_hook) + (*explain_per_plan_hook) (queryDesc->plannedstmt, + NULL, /* into */ + es, + debug_query_string, + queryDesc->params, + queryDesc->estate->es_queryEnv); ExplainEndOutput(es); /* Remove last line break */ diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 15c868021e6..91908161e09 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -299,6 +299,25 @@ LOAD 'auto_explain'; </para> </listitem> </varlistentry> + + <varlistentry id="auto-explain-configuration-parameters-log-options"> + <term> + <varname>auto_explain.log_options</varname> (<type>string</type>) + <indexterm> + <primary><varname>auto_explain.log_options</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + <varname>auto_explain.log_options</varname> specifies a comma-separated list + of custom <command>EXPLAIN</command> options registered by extensions. + This allows <filename>auto_explain</filename> to include output from + extension-provided <command>EXPLAIN</command> options when logging query + plans. The default is an empty string, meaning no custom options are + enabled. Only superusers can change this setting. + </para> + </listitem> + </varlistentry> </variablelist> <para> -- 2.52.0 Attachments: [text/plain] v1-0001-Add-custom-EXPLAIN-options-support-to-auto_explai.patch (9.6K, 2-v1-0001-Add-custom-EXPLAIN-options-support-to-auto_explai.patch) download | inline diff: From d493f789d50637574d186802ebe5937e2659341a Mon Sep 17 00:00:00 2001 From: Matheus Alcantara <[email protected]> Date: Thu, 26 Mar 2026 12:54:15 -0300 Subject: [PATCH v1] Add custom EXPLAIN options support to auto_explain Add a new GUC parameter auto_explain.log_options that accepts a comma-separated list of custom EXPLAIN options registered by extensions. This allows auto_explain to pass extension specific options (like those from pg_plan_advice) when logging query plans. Extensions that register custom EXPLAIN options follow two patterns: 1. Some extensions (e.g., pg_plan_advice) check during planning whether their custom option is enabled, to decide if extra information should be collected. To support this, a new planner_setup_hook is added that creates an ExplainState with the configured options. 2. Other extensions (e.g., pg_overexplain) only check at output time whether their option is enabled, using explain_per_plan_hook. Normally, explain_per_plan_hook is called by ExplainOnePlan() during a regular EXPLAIN command. However, auto_explain generates explain output manually at ExecutorEnd by calling ExplainPrintPlan and related functions directly, since ExplainOnePlan expects to execute the query itself. Therefore, this commit adds an explicit call to explain_per_plan_hook in explain_ExecutorEnd to allow extensions to include their output. --- contrib/auto_explain/auto_explain.c | 157 ++++++++++++++++++++++++++++ doc/src/sgml/auto-explain.sgml | 19 ++++ 2 files changed, 176 insertions(+) diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index e856cd35a6f..ff794394306 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -20,7 +20,11 @@ #include "commands/explain_state.h" #include "common/pg_prng.h" #include "executor/instrument.h" +#include "nodes/makefuncs.h" +#include "optimizer/planner.h" +#include "tcop/tcopprot.h" #include "utils/guc.h" +#include "utils/varlena.h" PG_MODULE_MAGIC_EXT( .name = "auto_explain", @@ -41,6 +45,7 @@ static int auto_explain_log_format = EXPLAIN_FORMAT_TEXT; static int auto_explain_log_level = LOG; static bool auto_explain_log_nested_statements = false; static double auto_explain_sample_rate = 1; +static char *auto_explain_log_options = NULL; static const struct config_enum_entry format_options[] = { {"text", EXPLAIN_FORMAT_TEXT, false}, @@ -76,11 +81,15 @@ static bool current_query_sampled = false; current_query_sampled) /* Saved hook values */ +static planner_setup_hook_type prev_planner_setup = NULL; static ExecutorStart_hook_type prev_ExecutorStart = NULL; static ExecutorRun_hook_type prev_ExecutorRun = NULL; static ExecutorFinish_hook_type prev_ExecutorFinish = NULL; static ExecutorEnd_hook_type prev_ExecutorEnd = NULL; +static void explain_planner_setup(PlannerGlobal *glob, Query *parse, + const char *query_string, int cursorOptions, + double *tuple_fraction, ExplainState *es); static void explain_ExecutorStart(QueryDesc *queryDesc, int eflags); static void explain_ExecutorRun(QueryDesc *queryDesc, ScanDirection direction, @@ -88,6 +97,125 @@ static void explain_ExecutorRun(QueryDesc *queryDesc, static void explain_ExecutorFinish(QueryDesc *queryDesc); static void explain_ExecutorEnd(QueryDesc *queryDesc); +/* + * Apply custom EXPLAIN options from auto_explain.log_options to the + * ExplainState. + * + * This parses the comma-separated option list and calls + * ApplyExtensionExplainOption for each one. + */ +static void +apply_custom_options(ExplainState *es) +{ + char *options; + List *elemlist; + ListCell *lc; + + if (auto_explain_log_options == NULL || auto_explain_log_options[0] == '\0') + return; + + options = pstrdup(auto_explain_log_options); + + if (!SplitIdentifierString(options, ',', &elemlist)) + { + /* Shouldn't happen since check_explain_options validated this */ + Assert(false); + pfree(options); + return; + } + + foreach(lc, elemlist) + { + const char *option = (const char *) lfirst(lc); + DefElem *def; + + /* + * Create a DefElem for this option. Pass NULL as the argument, which + * for boolean options means "true". + */ + def = makeDefElem(pstrdup(option), NULL, -1); + + /* + * Apply the option. If the extension that registered this option is + * not loaded, ApplyExtensionExplainOption will return false, which we + * silently ignore. This allows the GUC to be set even if the + * extension providing the option isn't currently loaded. + * + * XXX: ParseState is not used by pg_plan_advice and pg_overexplain. + * Using NULL is a problem for future extensions? + */ + ApplyExtensionExplainOption(es, def, NULL); + } + + pfree(options); + list_free(elemlist); +} + +/* GUC check hook for auto_explain.log_options. */ +static bool +check_explain_options(char **newval, void **extra, GucSource source) +{ + char *options; + List *elemlist; + + if (*newval == NULL || (*newval)[0] == '\0') + return true; + + /* Make a modifiable copy */ + options = pstrdup(*newval); + + if (!SplitIdentifierString(options, ',', &elemlist)) + { + GUC_check_errdetail("Invalid syntax in option list."); + pfree(options); + list_free(elemlist); + return false; + } + + pfree(options); + list_free(elemlist); + return true; +} + +/* + * Planner setup hook: pass custom EXPLAIN options to planner extensions. + * + * Extensions like pg_plan_advice need to know about custom EXPLAIN options + * during planning so they can generate data that will be displayed later. + * When auto_explain.log_options is configured and auto_explain is potentially + * active, we create an ExplainState with those options and pass it to the + * planner hook chain. + */ +static void +explain_planner_setup(PlannerGlobal *glob, Query *parse, + const char *query_string, int cursorOptions, + double *tuple_fraction, ExplainState *es) +{ + /* + * If auto_explain is potentially active (log_min_duration >= 0) and we + * have custom options configured, create an ExplainState with those + * options applied. This signals to extensions like pg_plan_advice that + * they should generate data for these options. + * + * We do this even if the caller already provided an ExplainState (i.e., + * we're inside an EXPLAIN command), because our options might differ. + * However, if an ExplainState is already provided, extensions will see + * that one first, so we only create ours if es is NULL. + */ + if (auto_explain_log_min_duration >= 0 && + auto_explain_log_options != NULL && + auto_explain_log_options[0] != '\0') + { + if (es == NULL) + es = NewExplainState(); + + apply_custom_options(es); + } + + if (prev_planner_setup) + prev_planner_setup(glob, parse, query_string, cursorOptions, + tuple_fraction, es); +} /* * Module load callback @@ -245,9 +373,22 @@ _PG_init(void) NULL, NULL); + DefineCustomStringVariable("auto_explain.log_options", + "Custom EXPLAIN options to include in plan logging.", + "Comma-separated list of custom EXPLAIN options registered by extensions. ", + &auto_explain_log_options, + NULL, + PGC_SUSET, + GUC_LIST_INPUT, + check_explain_options, + NULL, + NULL); + MarkGUCPrefixReserved("auto_explain"); /* Install hooks. */ + prev_planner_setup = planner_setup_hook; + planner_setup_hook = explain_planner_setup; prev_ExecutorStart = ExecutorStart_hook; ExecutorStart_hook = explain_ExecutorStart; prev_ExecutorRun = ExecutorRun_hook; @@ -404,6 +545,9 @@ explain_ExecutorEnd(QueryDesc *queryDesc) es->format = auto_explain_log_format; es->settings = auto_explain_log_settings; + /* Apply any custom EXPLAIN options */ + apply_custom_options(es); + ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); ExplainQueryParameters(es, queryDesc->params, auto_explain_log_parameter_max_length); @@ -412,6 +556,19 @@ explain_ExecutorEnd(QueryDesc *queryDesc) ExplainPrintTriggers(es, queryDesc); if (es->costs) ExplainPrintJITSummary(es, queryDesc); + + /* + * Allow plugins to print additional EXPLAIN information. This + * mirrors what ExplainOnePlan does, allowing extensions that use + * explain_per_plan_hook to add their output. + */ + if (explain_per_plan_hook) + (*explain_per_plan_hook) (queryDesc->plannedstmt, + NULL, /* into */ + es, + debug_query_string, + queryDesc->params, + queryDesc->estate->es_queryEnv); ExplainEndOutput(es); /* Remove last line break */ diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 15c868021e6..91908161e09 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -299,6 +299,25 @@ LOAD 'auto_explain'; </para> </listitem> </varlistentry> + + <varlistentry id="auto-explain-configuration-parameters-log-options"> + <term> + <varname>auto_explain.log_options</varname> (<type>string</type>) + <indexterm> + <primary><varname>auto_explain.log_options</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + <varname>auto_explain.log_options</varname> specifies a comma-separated list + of custom <command>EXPLAIN</command> options registered by extensions. + This allows <filename>auto_explain</filename> to include output from + extension-provided <command>EXPLAIN</command> options when logging query + plans. The default is an empty string, meaning no custom options are + enabled. Only superusers can change this setting. + </para> + </listitem> + </varlistentry> </variablelist> <para> -- 2.52.0 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-03-30 21:49 Robert Haas <[email protected]> parent: Matheus Alcantara <[email protected]> 0 siblings, 1 reply; 16+ messages in thread From: Robert Haas @ 2026-03-30 21:49 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: [email protected] On Thu, Mar 26, 2026 at 3:18 PM Matheus Alcantara <[email protected]> wrote: > Attached patch add a new GUC parameter auto_explain.log_options that > accepts a comma-separated list of custom EXPLAIN options registered by > extensions. This allows auto_explain to pass extension specific options > (like from pg_plan_advice) when logging query plans. I like this idea a lot, but I don't think the details work. It seems to me that fabricating an ExplainState to pass down as this patch does is probably not very safe. I see why you're doing that: the built-in options can be signaled via instrument_options flags, but custom EXPLAIN options can only be requested via an ExplainState, which won't exist here unless the user happens to be running EXPLAIN, so you have to create one. But I think that might cause problems for other extensions that are expecting the ExplainState to exist only when the command being run is actually EXPLAIN, and it also makes the behavior dependent on the module load order. > That being said, this patch creates a new planner_setup_hook for case > 1 and changes explain_ExecutorEnd() to call explain_per_plan_hook() > for case 2. Note that even for case 1, we still need to call > explain_per_plan_hook() so the extra information from the custom > explain option is included in the explain output. I think that adding a call here to explain_per_plan_hook() makes a lot of sense. Doing some refactoring so that auto_explain can reuse some core function instead of rolling its own might make sense, too. In terms of making things work with pg_plan_advice, it seems to me that all we really need here is (1) add a call to explain_per_plan_hook() as you've done, or possibly with some refactoring, and (2) provide a way to set custom options in the ExplainState that is explain_ExecutorEnd is already creating. If had that much, then the user could set pg_plan_advice.always_store_advice_details=true to close the remaining gap. Any module other than pg_plan_advice that needs to behave differently at plan time depending on EXPLAIN options should provide a similar option, because the issue is fundamental. Modules that don't, like pg_overexplain, just work with no further changes. But, while (1) is simple, it seems that (2) is not. When you actually run EXPLAIN (options go here), any problems with those options will result in throwing an ERROR. But auto_explain does not want to have foreground queries start erroring out because of problems with its configuration. For built-in options, it can and does avoid that by validating those options at the time you set auto_explain.log_WHATEVER, and then it also just kind of makes an end-run around the cross-checks between different options. For custom options, that doesn't work, because RegisterExtensionExplainOption() registers a single handler that *both* throws errors if the option value isn't OK *and also* feeds required information into the ExplainState. I'm currently poking at some ideas for fixing this... more soon. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-03-31 16:18 Robert Haas <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Robert Haas @ 2026-03-31 16:18 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: [email protected]; Lukas Fittl <[email protected]>; Tom Lane <[email protected]> On Mon, Mar 30, 2026 at 5:49 PM Robert Haas <[email protected]> wrote: > I'm currently poking at some ideas for fixing this... more soon. Here are some patches. I got started poking at this in earnest because, on the pg_plan_advice thread, Lukas was saying that instead of adopting pg_collect_advice, we should just add an option to send advice strings for each executed query to the server log. I went to implement that and then felt like it should really be part of auto_explain rather than its own thing, which took me down a bit of a rathole. But I eventually found my way back out of it, so here's a patch set implementing auto_explain.log_extension_options. There were two main complications. One is that I didn't want to assume all options are Boolean. Right now, at least in core, they are: both pg_overexplain and pg_plan_advice only use Booleans. But, that might change in the future, or already be true in other extensions that other people have written. Also, we might at some point in the future want to consider folding the built-in options back into the same mechanism -- i.e. get rid of log_extension_options and a pile of the other parameters and just have log_options -- and then we'd need to handle FORMAT, at least. The other big complication is that right now there is no way to sanity check an EXPLAIN extension option except try to set it and see if things go boom. That would mean that if you were to configure auto_explain.log_extension_options='KANGAROO', the server would basically just go down. I mean, technically it would be up, but every query would ERROR when auto_explain tried to kangaroo-ify the EXPLAIN output. That seems not so nice. So I ended up with this: - 0001 does a bit of refactoring to expose some of the logic in SplitIdentifierString and similar functions in a way that allows for reuse. - 0002 modifies the "explain extension" option mechanism so that each explain extension option has an associated GUC check handler. - 0003 then uses those things to implement the feature One thing that I suspect Tom in particular may be unhappy about (and thus I'm copying him...) is that the validation framework in 0002 means that you have to load EXPLAIN-option providers before you can use their options in the value of the new GUC. We try pretty hard to make sure that the legal values for a GUC don't depend on the values of other GUCs, and this is technically not a violation of that principle because it's making the legal values for a GUC depend on module loading, not other GUCs, so maybe it's fine. But on the other hand, maybe it isn't. I'm not quite sure what the reasonable alternative is: I guess we could instead add a "soft apply" hook for EXPLAIN options where the contract is that the EXPLAIN option provider is to do nothing if the option is not valid rather than complaining, but then setting an invalid option wouldn't ever give any kind of error -- it would just not work. That seems pretty unpleasant. Anyway, if you apply all these patches it does solve the problem that pg_collect_advice was targeting, modulo the need for some log parsing. You can do this: pg_plan_advice.always_store_advice_details = on auto_explain.log_min_duration = 0 auto_explain.log_extension_options = 'plan_advice' And then you get log output like this: 2026-03-31 12:16:18.784 EDT [75224] LOG: duration: 0.013 ms plan: Query Text: select 1; Result (cost=0.00..0.01 rows=1 width=4) Generated Plan Advice: NO_GATHER("*RESULT*") Thoughts? -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v1-0003-auto_explain-Add-new-GUC-auto_explain.log_extensi.patch (15.5K, 2-v1-0003-auto_explain-Add-new-GUC-auto_explain.log_extensi.patch) download | inline diff: From 2a4e75d661f612400226ca21114ba18a308a688b Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Tue, 31 Mar 2026 11:48:48 -0400 Subject: [PATCH v1 3/3] auto_explain: Add new GUC, auto_explain.log_extension_options. The associated value should look like something that could be part of an EXPLAIN options list, but restricted to EXPLAIN options added by extensions. For example, if pg_overexplain is loaded, you could set auto_explain.log_extension_options = 'DEBUG, RANGE_TABLE'. You can also specify arguments to these options in the same manner as normal e.g. 'DEBUG 1, RANGE_TABLE false'. --- contrib/auto_explain/Makefile | 2 +- contrib/auto_explain/auto_explain.c | 378 ++++++++++++++++++++++++++++ contrib/auto_explain/meson.build | 1 + doc/src/sgml/auto-explain.sgml | 23 ++ src/tools/pgindent/typedefs.list | 2 + 5 files changed, 405 insertions(+), 1 deletion(-) diff --git a/contrib/auto_explain/Makefile b/contrib/auto_explain/Makefile index 94ab28e7c06..8adaba0c3ab 100644 --- a/contrib/auto_explain/Makefile +++ b/contrib/auto_explain/Makefile @@ -6,7 +6,7 @@ OBJS = \ auto_explain.o PGFILEDESC = "auto_explain - logging facility for execution plans" -REGRESS = alter_reset +REGRESS = alter_reset extension_options TAP_TESTS = 1 diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index e856cd35a6f..132c3025f33 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -15,12 +15,17 @@ #include <limits.h> #include "access/parallel.h" +#include "commands/defrem.h" #include "commands/explain.h" #include "commands/explain_format.h" #include "commands/explain_state.h" #include "common/pg_prng.h" #include "executor/instrument.h" +#include "nodes/makefuncs.h" +#include "nodes/value.h" +#include "parser/scansup.h" #include "utils/guc.h" +#include "utils/varlena.h" PG_MODULE_MAGIC_EXT( .name = "auto_explain", @@ -41,6 +46,31 @@ static int auto_explain_log_format = EXPLAIN_FORMAT_TEXT; static int auto_explain_log_level = LOG; static bool auto_explain_log_nested_statements = false; static double auto_explain_sample_rate = 1; +static char *auto_explain_log_extension_options = NULL; + +/* + * Parsed form of one option from auto_explain.log_extension_options. + */ +typedef struct auto_explain_option +{ + char *name; + char *value; + NodeTag type; +} auto_explain_option; + +/* + * Parsed form of the entirety of auto_explain.log_extension_options, stored + * as GUC extra. The options[] array will have pointers into the string + * following the array. + */ +typedef struct auto_explain_extension_options +{ + int noptions; + auto_explain_option options[FLEXIBLE_ARRAY_MEMBER]; + /* a null-terminated copy of the GUC string follows the array */ +} auto_explain_extension_options; + +static auto_explain_extension_options *extension_options = NULL; static const struct config_enum_entry format_options[] = { {"text", EXPLAIN_FORMAT_TEXT, false}, @@ -88,6 +118,15 @@ static void explain_ExecutorRun(QueryDesc *queryDesc, static void explain_ExecutorFinish(QueryDesc *queryDesc); static void explain_ExecutorEnd(QueryDesc *queryDesc); +static bool check_log_extension_options(char **newval, void **extra, + GucSource source); +static void assign_log_extension_options(const char *newval, void *extra); +static void apply_extension_options(ExplainState *es, + auto_explain_extension_options *ext); +static char *auto_explain_scan_literal(char **endp, char **nextp); +static int auto_explain_split_options(char *rawstring, + auto_explain_option *options, + int maxoptions, char **errmsg); /* * Module load callback @@ -232,6 +271,17 @@ _PG_init(void) NULL, NULL); + DefineCustomStringVariable("auto_explain.log_extension_options", + "Extension EXPLAIN options to be added.", + NULL, + &auto_explain_log_extension_options, + NULL, + PGC_SUSET, + 0, + check_log_extension_options, + assign_log_extension_options, + NULL); + DefineCustomRealVariable("auto_explain.sample_rate", "Fraction of queries to process.", NULL, @@ -404,6 +454,8 @@ explain_ExecutorEnd(QueryDesc *queryDesc) es->format = auto_explain_log_format; es->settings = auto_explain_log_settings; + apply_extension_options(es, extension_options); + ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); ExplainQueryParameters(es, queryDesc->params, auto_explain_log_parameter_max_length); @@ -412,6 +464,12 @@ explain_ExecutorEnd(QueryDesc *queryDesc) ExplainPrintTriggers(es, queryDesc); if (es->costs) ExplainPrintJITSummary(es, queryDesc); + if (explain_per_plan_hook) + (*explain_per_plan_hook) (queryDesc->plannedstmt, + NULL, es, + queryDesc->sourceText, + queryDesc->params, + queryDesc->estate->es_queryEnv); ExplainEndOutput(es); /* Remove last line break */ @@ -445,3 +503,323 @@ explain_ExecutorEnd(QueryDesc *queryDesc) else standard_ExecutorEnd(queryDesc); } + +/* + * GUC check hook for auto_explain.log_extension_options. + */ +static bool +check_log_extension_options(char **newval, void **extra, GucSource source) +{ + char *rawstring; + auto_explain_extension_options *result; + auto_explain_option *options; + int maxoptions = 8; + Size rawstring_len; + Size allocsize; + char *errmsg; + + /* NULL or empty string means no options. */ + if (*newval == NULL || (*newval)[0] == '\0') + { + *extra = NULL; + return true; + } + + rawstring_len = strlen(*newval) + 1; + +retry: + /* Try to allocate an auto_explain_extension_options object. */ + allocsize = offsetof(auto_explain_extension_options, options) + + sizeof(auto_explain_option) * maxoptions + + rawstring_len; + result = (auto_explain_extension_options *) guc_malloc(LOG, allocsize); + if (result == NULL) + return false; + + /* Copy the string after the options array. */ + rawstring = (char *) &result->options[maxoptions]; + memcpy(rawstring, *newval, rawstring_len); + + /* Parse. */ + options = result->options; + result->noptions = auto_explain_split_options(rawstring, options, + maxoptions, &errmsg); + if (result->noptions < 0) + { + GUC_check_errdetail("%s", errmsg); + guc_free(result); + return false; + } + + /* + * Retry with a larger array if needed. + * + * It should be impossible for this to loop more than once, because + * auto_explain_split_options tells us how many entries are needed. + */ + if (result->noptions > maxoptions) + { + maxoptions = result->noptions; + guc_free(result); + goto retry; + } + + /* Validate each option against its registered check handler. */ + for (int i = 0; i < result->noptions; i++) + { + if (!GUCCheckExplainExtensionOption(options[i].name, options[i].value, + options[i].type)) + { + guc_free(result); + return false; + } + } + + *extra = result; + return true; +} + +/* + * GUC assign hook for auto_explain.log_extension_options. + */ +static void +assign_log_extension_options(const char *newval, void *extra) +{ + extension_options = (auto_explain_extension_options *) extra; +} + +/* + * Apply parsed extension options to an ExplainState. + */ +static void +apply_extension_options(ExplainState *es, auto_explain_extension_options *ext) +{ + if (ext == NULL) + return; + + for (int i = 0; i < ext->noptions; i++) + { + auto_explain_option *opt = &ext->options[i]; + DefElem *def; + Node *arg; + + if (opt->value == NULL) + arg = NULL; + else if (opt->type == T_Integer) + arg = (Node *) makeInteger(strtol(opt->value, NULL, 0)); + else if (opt->type == T_Float) + arg = (Node *) makeFloat(opt->value); + else + arg = (Node *) makeString(opt->value); + + def = makeDefElem(opt->name, arg, -1); + ApplyExtensionExplainOption(es, def, NULL); + } +} + +/* + * auto_explain_scan_literal - In-place scanner for single-quoted string + * literals. + * + * This is the single-quote analog of scan_quoted_identifier from varlena.c. + */ +static char * +auto_explain_scan_literal(char **endp, char **nextp) +{ + char *token = *nextp + 1; + + for (;;) + { + *endp = strchr(*nextp + 1, '\''); + if (*endp == NULL) + return NULL; /* mismatched quotes */ + if ((*endp)[1] != '\'') + break; /* found end of literal */ + /* Collapse adjacent quotes into one quote, and look again */ + memmove(*endp, *endp + 1, strlen(*endp)); + *nextp = *endp; + } + /* *endp now points at the terminating quote */ + *nextp = *endp + 1; + + return token; +} + +/* + * auto_explain_split_options - Parse an option string into an array of + * auto_explain_option structs. + * + * Much of this logic is similar to SplitIdentifierString and friends, but our + * needs are different enough that we roll our own parsing logic. The goal here + * is to accept the same syntax that the main parser would accepts inside of + * an EXPLAIN option list. While we can't do that perfectly without adding a + * lot more code, the goal of this implementation is to be close enough that + * users don't really notice the differences. + * + * The input string is modified in place (null-terminated, downcased, quotes + * collapsed). All name and value pointers in the output array refer into + * this string, so the caller must ensure the string outlives the array. + * + * Returns the full number of options in the input string, but stores no + * more than maxoptions into the caller-provided array. If a syntax error + * occurs, returns -1 and sets *errmsg. + */ +static int +auto_explain_split_options(char *rawstring, auto_explain_option *options, + int maxoptions, char **errmsg) +{ + char *nextp = rawstring; + int noptions = 0; + bool done = false; + + *errmsg = NULL; + + while (scanner_isspace(*nextp)) + nextp++; /* skip leading whitespace */ + + if (*nextp == '\0') + return 0; /* empty string is fine */ + + while (!done) + { + char *name; + char *name_endp; + char *value = NULL; + char *value_endp = NULL; + NodeTag type = T_Invalid; + + /* Parse the option name. */ + name = scan_identifier(&name_endp, &nextp, ',', true); + if (name == NULL || name_endp == name) + { + *errmsg = "option name missing or empty"; + return -1; + } + + /* Skip whitespace after the option name. */ + while (scanner_isspace(*nextp)) + nextp++; + + /* + * Determine whether we have an option value. A comma or end of + * string means no value; otherwise we have one. + */ + if (*nextp != '\0' && *nextp != ',') + { + if (*nextp == '\'') + { + /* Single-quoted string literal. */ + type = T_String; + value = auto_explain_scan_literal(&value_endp, &nextp); + if (value == NULL) + { + *errmsg = "unterminated single-quoted string"; + return -1; + } + } + else if (isdigit((unsigned char) *nextp) || + ((*nextp == '+' || *nextp == '-') && + isdigit((unsigned char) nextp[1]))) + { + char *endptr; + long intval; + char saved; + + /* Remember the start of the next token, and find the end. */ + value = nextp; + while (*nextp && *nextp != ',' && !scanner_isspace(*nextp)) + nextp++; + value_endp = nextp; + + /* Temporarily '\0'-terminate so we can use strtol/strtod. */ + saved = *value_endp; + *value_endp = '\0'; + + /* Integer, float, or neither? */ + errno = 0; + intval = strtol(value, &endptr, 0); + if (errno == 0 && *endptr == '\0' && endptr != value && + intval == (int) intval) + type = T_Integer; + else + { + type = T_Float; + (void) strtod(value, &endptr); + if (*endptr != '\0') + { + *value_endp = saved; + *errmsg = "invalid numeric value"; + return -1; + } + } + + /* Remove temporary terminator. */ + *value_endp = saved; + } + else + { + /* Identifier, possibly double-quoted. */ + type = T_String; + value = scan_identifier(&value_endp, &nextp, ',', true); + if (value == NULL) + { + /* + * scan_identifier will return NULL if it finds an + * unterminated double-quoted identifier or it finds no + * identifier at all because the next character is + * whitespace or the separator character, here a comma. + * But the latter case is impossible here because the code + * above has skipped whitespace and checked for commas. + */ + *errmsg = "unterminated double-quoted string"; + return -1; + } + } + } + + /* Skip trailing whitespace. */ + while (scanner_isspace(*nextp)) + nextp++; + + /* Expect comma or end of string. */ + if (*nextp == ',') + { + nextp++; + while (scanner_isspace(*nextp)) + nextp++; + if (*nextp == '\0') + { + *errmsg = "trailing comma in option list"; + return -1; + } + } + else if (*nextp == '\0') + done = true; + else + { + *errmsg = "expected comma or end of option list"; + return -1; + } + + /* + * Now safe to null-terminate the name and value. We couldn't do this + * earlier because in the unquoted case, the null terminator position + * may coincide with a character that the scanning logic above still + * needed to read. + */ + *name_endp = '\0'; + if (value_endp != NULL) + *value_endp = '\0'; + + /* Always count this option, and store the details if there is room. */ + if (noptions < maxoptions) + { + options[noptions].name = name; + options[noptions].type = type; + options[noptions].value = value; + } + noptions++; + } + + return noptions; +} diff --git a/contrib/auto_explain/meson.build b/contrib/auto_explain/meson.build index 6f9d22bf5d8..d2b0650af1c 100644 --- a/contrib/auto_explain/meson.build +++ b/contrib/auto_explain/meson.build @@ -23,6 +23,7 @@ tests += { 'regress': { 'sql': [ 'alter_reset', + 'extension_options', ], }, 'tap': { diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 15c868021e6..ee85a67eb2e 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -245,6 +245,29 @@ LOAD 'auto_explain'; </listitem> </varlistentry> + <varlistentry id="auto-explain-configuration-parameters-log-extension-options"> + <term> + <varname>auto_explain.log_extension_options</varname> (<type>string</type>) + <indexterm> + <primary><varname>auto_explain.log_extension_options</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Loadable modules can extend the <literal>EXPLAIN</literal> command with + additional options that affect the output format. Such options can also + be specified here. The value of this parameter is a comma-separated + list of options, each of which is an option name followed optionally by + an associated value. The module that provides the + <literal>EXPLAIN</literal> option, such as + <link linkend="pgplanadvice"><literal>pg_plan_advice</literal></link> or + <link linkend="pgoverexplain"><literal>pg_overexplain</literal></link>, + should be loaded before this parameter is set. + Only superusers can change this setting. + </para> + </listitem> + </varlistentry> + <varlistentry id="auto-explain-configuration-parameters-log-level"> <term> <varname>auto_explain.log_level</varname> (<type>enum</type>) diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 2e4e913331b..c602ae2d690 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3561,6 +3561,8 @@ astreamer_verify astreamer_waldump astreamer_zstd_frame auth_password_hook_typ +auto_explain_extension_options +auto_explain_option autovac_table av_relation avc_cache -- 2.51.0 [application/octet-stream] v1-0001-Expose-helper-functions-scan_quoted_identifier-an.patch (8.3K, 3-v1-0001-Expose-helper-functions-scan_quoted_identifier-an.patch) download | inline diff: From d33f6ab7bf00465383c414e4f0ed56ff062ef7af Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Mon, 30 Mar 2026 18:49:56 -0400 Subject: [PATCH v1 1/3] Expose helper functions scan_quoted_identifier and scan_identifier. Previously, this logic was embedded within SplitIdentifierString, SplitDirectoriesString, and SplitGUCList. Factoring it out saves a bit of duplicated code, and also makes it available to extensions that might want to do similar things without necessarily wanting to do exactly the same thing. --- src/backend/utils/adt/varlena.c | 189 +++++++++++++++++--------------- src/include/utils/varlena.h | 3 + 2 files changed, 102 insertions(+), 90 deletions(-) diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index 7b1ee61bde6..5816df6587a 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -2750,6 +2750,96 @@ textToQualifiedNameList(text *textval) return result; } +/* + * scan_quoted_identifier - In-place scanner for quoted identifiers. + * + * *nextp should point to the opening double-quote character, and will be + * updated to point just past the end. *endp is set to the position of + * the closing quote. The return value is the identifier, or NULL if the + * matching close-quote cannot be found. + * + * If we find two consecutive double quote characters, that doesn't end the + * identifier: instead, we collapse them into a double quote and include them + * in the resulting token. Note that this requires overwriting the rest of the + * string in place, including the portion beyond the final value of *nextp. + */ +char * +scan_quoted_identifier(char **endp, char **nextp) +{ + char *token = *nextp + 1; + + for (;;) + { + *endp = strchr(*nextp + 1, '"'); + if (*endp == NULL) + return NULL; /* mismatched quotes */ + if ((*endp)[1] != '"') + break; /* found end of quoted identifier */ + /* Collapse adjacent quotes into one quote, and look again */ + memmove(*endp, *endp + 1, strlen(*endp)); + *nextp = *endp; + } + /* *endp now points at the terminating quote */ + *nextp = *endp + 1; + + return token; +} + +/* + * scan_identifier - In-place scanner for quoted or unquoted identifiers. + * + * On success, *endp is set to the position where the caller should write '\0' + * to null-terminate the token, and *nextp is advanced past the token (and past + * the closing quote, if any). The return value is the token content, or NULL + * if there is a syntax error (mismatched quotes or empty unquoted token). + * + * Unquoted identifiers are terminated by whitespace or the first occurrence + * of the separator character. Additionally, if downcase_unquoted = true, + * unquoted identifiers are downcased in place. See scan_quoted_identifier for + * an additional way in which we modify the string in place. + */ +char * +scan_identifier(char **endp, char **nextp, char separator, bool downcase_unquoted) +{ + char *token; + + if (**nextp == '"') + return scan_quoted_identifier(endp, nextp); + + /* Unquoted identifier --- extends to separator or whitespace */ + token = *nextp; + + while (**nextp && **nextp != separator && !scanner_isspace(**nextp)) + (*nextp)++; + + if (*nextp == token) + return NULL; /* empty token */ + + *endp = *nextp; + + if (downcase_unquoted) + { + /* + * Downcase the identifier, using same code as main lexer does. + * + * XXX because we want to overwrite the input in-place, we cannot + * support a downcasing transformation that increases the string + * length. This is not a problem given the current implementation of + * downcase_truncate_identifier, but we'll probably have to do + * something about this someday. + */ + int len = *endp - token; + char *downname = downcase_truncate_identifier(token, len, false); + + Assert(strlen(downname) <= len); + strncpy(token, downname, len); /* strncpy is required here */ + pfree(downname); + } + + return token; +} + + /* * SplitIdentifierString --- parse a string containing identifiers * @@ -2794,53 +2884,9 @@ SplitIdentifierString(char *rawstring, char separator, char *curname; char *endp; - if (*nextp == '"') - { - /* Quoted name --- collapse quote-quote pairs, no downcasing */ - curname = nextp + 1; - for (;;) - { - endp = strchr(nextp + 1, '"'); - if (endp == NULL) - return false; /* mismatched quotes */ - if (endp[1] != '"') - break; /* found end of quoted name */ - /* Collapse adjacent quotes into one quote, and look again */ - memmove(endp, endp + 1, strlen(endp)); - nextp = endp; - } - /* endp now points at the terminating quote */ - nextp = endp + 1; - } - else - { - /* Unquoted name --- extends to separator or whitespace */ - char *downname; - int len; - - curname = nextp; - while (*nextp && *nextp != separator && - !scanner_isspace(*nextp)) - nextp++; - endp = nextp; - if (curname == nextp) - return false; /* empty unquoted name not allowed */ - - /* - * Downcase the identifier, using same code as main lexer does. - * - * XXX because we want to overwrite the input in-place, we cannot - * support a downcasing transformation that increases the string - * length. This is not a problem given the current implementation - * of downcase_truncate_identifier, but we'll probably have to do - * something about this someday. - */ - len = endp - curname; - downname = downcase_truncate_identifier(curname, len, false); - Assert(strlen(downname) <= len); - strncpy(curname, downname, len); /* strncpy is required here */ - pfree(downname); - } + curname = scan_identifier(&endp, &nextp, separator, true); + if (curname == NULL) + return false; /* mismatched quotes or empty name */ while (scanner_isspace(*nextp)) nextp++; /* skip trailing whitespace */ @@ -2924,20 +2970,9 @@ SplitDirectoriesString(char *rawstring, char separator, if (*nextp == '"') { /* Quoted name --- collapse quote-quote pairs */ - curname = nextp + 1; - for (;;) - { - endp = strchr(nextp + 1, '"'); - if (endp == NULL) - return false; /* mismatched quotes */ - if (endp[1] != '"') - break; /* found end of quoted name */ - /* Collapse adjacent quotes into one quote, and look again */ - memmove(endp, endp + 1, strlen(endp)); - nextp = endp; - } - /* endp now points at the terminating quote */ - nextp = endp + 1; + curname = scan_quoted_identifier(&endp, &nextp); + if (curname == NULL) + return false; /* mismatched quotes */ } else { @@ -3042,35 +3077,9 @@ SplitGUCList(char *rawstring, char separator, char *curname; char *endp; - if (*nextp == '"') - { - /* Quoted name --- collapse quote-quote pairs */ - curname = nextp + 1; - for (;;) - { - endp = strchr(nextp + 1, '"'); - if (endp == NULL) - return false; /* mismatched quotes */ - if (endp[1] != '"') - break; /* found end of quoted name */ - /* Collapse adjacent quotes into one quote, and look again */ - memmove(endp, endp + 1, strlen(endp)); - nextp = endp; - } - /* endp now points at the terminating quote */ - nextp = endp + 1; - } - else - { - /* Unquoted name --- extends to separator or whitespace */ - curname = nextp; - while (*nextp && *nextp != separator && - !scanner_isspace(*nextp)) - nextp++; - endp = nextp; - if (curname == nextp) - return false; /* empty unquoted name not allowed */ - } + curname = scan_identifier(&endp, &nextp, separator, false); + if (curname == NULL) + return false; /* mismatched quotes or empty name */ while (scanner_isspace(*nextp)) nextp++; /* skip trailing whitespace */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index 4b32574a075..fe8d8a58952 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -27,6 +27,9 @@ extern int varstr_levenshtein_less_equal(const char *source, int slen, int ins_c, int del_c, int sub_c, int max_d, bool trusted); extern List *textToQualifiedNameList(text *textval); +extern char *scan_quoted_identifier(char **endp, char **nextp); +extern char *scan_identifier(char **endp, char **nextp, char separator, + bool downcase_unquoted); extern bool SplitIdentifierString(char *rawstring, char separator, List **namelist); extern bool SplitDirectoriesString(char *rawstring, char separator, -- 2.51.0 [application/octet-stream] v1-0002-Add-a-guc_check_handler-to-the-EXPLAIN-extension-.patch (9.6K, 4-v1-0002-Add-a-guc_check_handler-to-the-EXPLAIN-extension-.patch) download | inline diff: From 7f671c41039183907e39b5302b6cd5751553a837 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Mon, 30 Mar 2026 19:10:32 -0400 Subject: [PATCH v1 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 thent 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. --- 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 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-03-31 22:10 Matheus Alcantara <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Matheus Alcantara @ 2026-03-31 22:10 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: [email protected]; Lukas Fittl <[email protected]>; Tom Lane <[email protected]> On Tue Mar 31, 2026 at 1:18 PM -03, Robert Haas wrote: > On Mon, Mar 30, 2026 at 5:49 PM Robert Haas <[email protected]> wrote: >> I'm currently poking at some ideas for fixing this... more soon. > > Here are some patches. I got started poking at this in earnest > because, on the pg_plan_advice thread, Lukas was saying that instead > of adopting pg_collect_advice, we should just add an option to send > advice strings for each executed query to the server log. I went to > implement that and then felt like it should really be part of > auto_explain rather than its own thing, which took me down a bit of a > rathole. But I eventually found my way back out of it, so here's a > patch set implementing auto_explain.log_extension_options. > > ... > > So I ended up with this: > > ... > > Thoughts? Hi, thanks for the patches. I think that the architecture is much better now. For 0001 I don't have any comment, it looks good to me. The 0002 also looks good, just a typo on "thent" on commit message. Some comments on 0003: + else if (opt->type == T_Integer) + arg = (Node *) makeInteger(strtol(opt->value, NULL, 0)); I think that we are safe against overflow because on auto_explain_split_options() it has intval == (int) intval, but I'm wondering if it's worth documenting this? ----- extension_options is being added to REGRESS in both Makefile and meson.build, but the actual test files are not included. ----- + an associated value. The module that provides the + <literal>EXPLAIN</literal> option, such as + <link linkend="pgplanadvice"><literal>pg_plan_advice</literal></link> or + <link linkend="pgoverexplain"><literal>pg_overexplain</literal></link>, + should be loaded before this parameter is set. Wondering if this is clear enough about the shared_preload_libraries order (auto_explain should be loaded after extensions that include explain options) or if we should mention this explicitly. -- Matheus Alcantara EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-02 18:17 Robert Haas <[email protected]> parent: Matheus Alcantara <[email protected]> 0 siblings, 2 replies; 16+ messages in thread From: Robert Haas @ 2026-04-02 18:17 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: [email protected]; Lukas Fittl <[email protected]>; Tom Lane <[email protected]> On Tue, Mar 31, 2026 at 6:10 PM Matheus Alcantara <[email protected]> wrote: > I think that we are safe against overflow because on > auto_explain_split_options() it has intval == (int) intval, but I'm > wondering if it's worth documenting this? We could add a comment here that the validity checks have already been done at an earlier stage, but I felt like that was overkill. Possibly not? > extension_options is being added to REGRESS in both Makefile and > meson.build, but the actual test files are not included. Well, that sucks. I accidentally erased that file instead of committing it. Here's a new version with mostly the same tests, plus I updated the TAP test with a related test case as well. > + an associated value. The module that provides the > + <literal>EXPLAIN</literal> option, such as > + <link linkend="pgplanadvice"><literal>pg_plan_advice</literal></link> or > + <link linkend="pgoverexplain"><literal>pg_overexplain</literal></link>, > + should be loaded before this parameter is set. > > Wondering if this is clear enough about the shared_preload_libraries > order (auto_explain should be loaded after extensions that include > explain options) or if we should mention this explicitly. I actually thought that this might not be true until I tested it and found that it sort of is. If you don't set auto_explain.log_extension_options in postgresql.conf, then you can load the modules in either order and it's fine. But if you do set it, then you need to have the EXPLAIN option provider before auto_explain, or else you get something like this: 2026-04-02 14:03:19.282 EDT [4614] WARNING: unrecognized EXPLAIN option "debug" ...because we read the whole postgresql.conf file before applying any of it, and so if auto_explain's _PG_init() runs first, the GUC value is already visible but the EXPLAIN option doesn't exist yet. That's annoying, but I'm not sure it's worth any more of a documentation reference than what I have already. "Before this parameter is set" could be read to encompass "put it earlier in shared_preload_libraries," and if someone does it wrong, it will become obvious quickly enough. If you (or someone else) doesn't agree, feel free to propose better wording -- I just don't want to expand the description so much that it becomes a distraction. -- Robert Haas EDB: http://www.enterprisedb.com Attachments: [application/octet-stream] v2-0003-auto_explain-Add-new-GUC-auto_explain.log_extensi.patch (22.1K, 2-v2-0003-auto_explain-Add-new-GUC-auto_explain.log_extensi.patch) download | inline diff: From 0be9723303dce4334cac593883e5e0a3c4a60182 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Thu, 2 Apr 2026 13:56:26 -0400 Subject: [PATCH v2 3/3] auto_explain: Add new GUC, auto_explain.log_extension_options. The associated value should look like something that could be part of an EXPLAIN options list, but restricted to EXPLAIN options added by extensions. For example, if pg_overexplain is loaded, you could set auto_explain.log_extension_options = 'DEBUG, RANGE_TABLE'. You can also specify arguments to these options in the same manner as normal e.g. 'DEBUG 1, RANGE_TABLE false'. Reviewed-by: Matheus Alcantara <[email protected]> Discussion: http://postgr.es/m/CA+Tgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w@mail.gmail.com --- contrib/auto_explain/Makefile | 3 +- contrib/auto_explain/auto_explain.c | 378 ++++++++++++++++++ .../expected/extension_options.out | 49 +++ contrib/auto_explain/meson.build | 1 + .../auto_explain/sql/extension_options.sql | 33 ++ contrib/auto_explain/t/001_auto_explain.pl | 18 +- doc/src/sgml/auto-explain.sgml | 23 ++ src/tools/pgindent/typedefs.list | 2 + 8 files changed, 505 insertions(+), 2 deletions(-) create mode 100644 contrib/auto_explain/expected/extension_options.out create mode 100644 contrib/auto_explain/sql/extension_options.sql diff --git a/contrib/auto_explain/Makefile b/contrib/auto_explain/Makefile index 94ab28e7c06..1f608b1d733 100644 --- a/contrib/auto_explain/Makefile +++ b/contrib/auto_explain/Makefile @@ -6,7 +6,8 @@ OBJS = \ auto_explain.o PGFILEDESC = "auto_explain - logging facility for execution plans" -REGRESS = alter_reset +EXTRA_INSTALL = contrib/pg_overexplain +REGRESS = alter_reset extension_options TAP_TESTS = 1 diff --git a/contrib/auto_explain/auto_explain.c b/contrib/auto_explain/auto_explain.c index e856cd35a6f..132c3025f33 100644 --- a/contrib/auto_explain/auto_explain.c +++ b/contrib/auto_explain/auto_explain.c @@ -15,12 +15,17 @@ #include <limits.h> #include "access/parallel.h" +#include "commands/defrem.h" #include "commands/explain.h" #include "commands/explain_format.h" #include "commands/explain_state.h" #include "common/pg_prng.h" #include "executor/instrument.h" +#include "nodes/makefuncs.h" +#include "nodes/value.h" +#include "parser/scansup.h" #include "utils/guc.h" +#include "utils/varlena.h" PG_MODULE_MAGIC_EXT( .name = "auto_explain", @@ -41,6 +46,31 @@ static int auto_explain_log_format = EXPLAIN_FORMAT_TEXT; static int auto_explain_log_level = LOG; static bool auto_explain_log_nested_statements = false; static double auto_explain_sample_rate = 1; +static char *auto_explain_log_extension_options = NULL; + +/* + * Parsed form of one option from auto_explain.log_extension_options. + */ +typedef struct auto_explain_option +{ + char *name; + char *value; + NodeTag type; +} auto_explain_option; + +/* + * Parsed form of the entirety of auto_explain.log_extension_options, stored + * as GUC extra. The options[] array will have pointers into the string + * following the array. + */ +typedef struct auto_explain_extension_options +{ + int noptions; + auto_explain_option options[FLEXIBLE_ARRAY_MEMBER]; + /* a null-terminated copy of the GUC string follows the array */ +} auto_explain_extension_options; + +static auto_explain_extension_options *extension_options = NULL; static const struct config_enum_entry format_options[] = { {"text", EXPLAIN_FORMAT_TEXT, false}, @@ -88,6 +118,15 @@ static void explain_ExecutorRun(QueryDesc *queryDesc, static void explain_ExecutorFinish(QueryDesc *queryDesc); static void explain_ExecutorEnd(QueryDesc *queryDesc); +static bool check_log_extension_options(char **newval, void **extra, + GucSource source); +static void assign_log_extension_options(const char *newval, void *extra); +static void apply_extension_options(ExplainState *es, + auto_explain_extension_options *ext); +static char *auto_explain_scan_literal(char **endp, char **nextp); +static int auto_explain_split_options(char *rawstring, + auto_explain_option *options, + int maxoptions, char **errmsg); /* * Module load callback @@ -232,6 +271,17 @@ _PG_init(void) NULL, NULL); + DefineCustomStringVariable("auto_explain.log_extension_options", + "Extension EXPLAIN options to be added.", + NULL, + &auto_explain_log_extension_options, + NULL, + PGC_SUSET, + 0, + check_log_extension_options, + assign_log_extension_options, + NULL); + DefineCustomRealVariable("auto_explain.sample_rate", "Fraction of queries to process.", NULL, @@ -404,6 +454,8 @@ explain_ExecutorEnd(QueryDesc *queryDesc) es->format = auto_explain_log_format; es->settings = auto_explain_log_settings; + apply_extension_options(es, extension_options); + ExplainBeginOutput(es); ExplainQueryText(es, queryDesc); ExplainQueryParameters(es, queryDesc->params, auto_explain_log_parameter_max_length); @@ -412,6 +464,12 @@ explain_ExecutorEnd(QueryDesc *queryDesc) ExplainPrintTriggers(es, queryDesc); if (es->costs) ExplainPrintJITSummary(es, queryDesc); + if (explain_per_plan_hook) + (*explain_per_plan_hook) (queryDesc->plannedstmt, + NULL, es, + queryDesc->sourceText, + queryDesc->params, + queryDesc->estate->es_queryEnv); ExplainEndOutput(es); /* Remove last line break */ @@ -445,3 +503,323 @@ explain_ExecutorEnd(QueryDesc *queryDesc) else standard_ExecutorEnd(queryDesc); } + +/* + * GUC check hook for auto_explain.log_extension_options. + */ +static bool +check_log_extension_options(char **newval, void **extra, GucSource source) +{ + char *rawstring; + auto_explain_extension_options *result; + auto_explain_option *options; + int maxoptions = 8; + Size rawstring_len; + Size allocsize; + char *errmsg; + + /* NULL or empty string means no options. */ + if (*newval == NULL || (*newval)[0] == '\0') + { + *extra = NULL; + return true; + } + + rawstring_len = strlen(*newval) + 1; + +retry: + /* Try to allocate an auto_explain_extension_options object. */ + allocsize = offsetof(auto_explain_extension_options, options) + + sizeof(auto_explain_option) * maxoptions + + rawstring_len; + result = (auto_explain_extension_options *) guc_malloc(LOG, allocsize); + if (result == NULL) + return false; + + /* Copy the string after the options array. */ + rawstring = (char *) &result->options[maxoptions]; + memcpy(rawstring, *newval, rawstring_len); + + /* Parse. */ + options = result->options; + result->noptions = auto_explain_split_options(rawstring, options, + maxoptions, &errmsg); + if (result->noptions < 0) + { + GUC_check_errdetail("%s", errmsg); + guc_free(result); + return false; + } + + /* + * Retry with a larger array if needed. + * + * It should be impossible for this to loop more than once, because + * auto_explain_split_options tells us how many entries are needed. + */ + if (result->noptions > maxoptions) + { + maxoptions = result->noptions; + guc_free(result); + goto retry; + } + + /* Validate each option against its registered check handler. */ + for (int i = 0; i < result->noptions; i++) + { + if (!GUCCheckExplainExtensionOption(options[i].name, options[i].value, + options[i].type)) + { + guc_free(result); + return false; + } + } + + *extra = result; + return true; +} + +/* + * GUC assign hook for auto_explain.log_extension_options. + */ +static void +assign_log_extension_options(const char *newval, void *extra) +{ + extension_options = (auto_explain_extension_options *) extra; +} + +/* + * Apply parsed extension options to an ExplainState. + */ +static void +apply_extension_options(ExplainState *es, auto_explain_extension_options *ext) +{ + if (ext == NULL) + return; + + for (int i = 0; i < ext->noptions; i++) + { + auto_explain_option *opt = &ext->options[i]; + DefElem *def; + Node *arg; + + if (opt->value == NULL) + arg = NULL; + else if (opt->type == T_Integer) + arg = (Node *) makeInteger(strtol(opt->value, NULL, 0)); + else if (opt->type == T_Float) + arg = (Node *) makeFloat(opt->value); + else + arg = (Node *) makeString(opt->value); + + def = makeDefElem(opt->name, arg, -1); + ApplyExtensionExplainOption(es, def, NULL); + } +} + +/* + * auto_explain_scan_literal - In-place scanner for single-quoted string + * literals. + * + * This is the single-quote analog of scan_quoted_identifier from varlena.c. + */ +static char * +auto_explain_scan_literal(char **endp, char **nextp) +{ + char *token = *nextp + 1; + + for (;;) + { + *endp = strchr(*nextp + 1, '\''); + if (*endp == NULL) + return NULL; /* mismatched quotes */ + if ((*endp)[1] != '\'') + break; /* found end of literal */ + /* Collapse adjacent quotes into one quote, and look again */ + memmove(*endp, *endp + 1, strlen(*endp)); + *nextp = *endp; + } + /* *endp now points at the terminating quote */ + *nextp = *endp + 1; + + return token; +} + +/* + * auto_explain_split_options - Parse an option string into an array of + * auto_explain_option structs. + * + * Much of this logic is similar to SplitIdentifierString and friends, but our + * needs are different enough that we roll our own parsing logic. The goal here + * is to accept the same syntax that the main parser would accepts inside of + * an EXPLAIN option list. While we can't do that perfectly without adding a + * lot more code, the goal of this implementation is to be close enough that + * users don't really notice the differences. + * + * The input string is modified in place (null-terminated, downcased, quotes + * collapsed). All name and value pointers in the output array refer into + * this string, so the caller must ensure the string outlives the array. + * + * Returns the full number of options in the input string, but stores no + * more than maxoptions into the caller-provided array. If a syntax error + * occurs, returns -1 and sets *errmsg. + */ +static int +auto_explain_split_options(char *rawstring, auto_explain_option *options, + int maxoptions, char **errmsg) +{ + char *nextp = rawstring; + int noptions = 0; + bool done = false; + + *errmsg = NULL; + + while (scanner_isspace(*nextp)) + nextp++; /* skip leading whitespace */ + + if (*nextp == '\0') + return 0; /* empty string is fine */ + + while (!done) + { + char *name; + char *name_endp; + char *value = NULL; + char *value_endp = NULL; + NodeTag type = T_Invalid; + + /* Parse the option name. */ + name = scan_identifier(&name_endp, &nextp, ',', true); + if (name == NULL || name_endp == name) + { + *errmsg = "option name missing or empty"; + return -1; + } + + /* Skip whitespace after the option name. */ + while (scanner_isspace(*nextp)) + nextp++; + + /* + * Determine whether we have an option value. A comma or end of + * string means no value; otherwise we have one. + */ + if (*nextp != '\0' && *nextp != ',') + { + if (*nextp == '\'') + { + /* Single-quoted string literal. */ + type = T_String; + value = auto_explain_scan_literal(&value_endp, &nextp); + if (value == NULL) + { + *errmsg = "unterminated single-quoted string"; + return -1; + } + } + else if (isdigit((unsigned char) *nextp) || + ((*nextp == '+' || *nextp == '-') && + isdigit((unsigned char) nextp[1]))) + { + char *endptr; + long intval; + char saved; + + /* Remember the start of the next token, and find the end. */ + value = nextp; + while (*nextp && *nextp != ',' && !scanner_isspace(*nextp)) + nextp++; + value_endp = nextp; + + /* Temporarily '\0'-terminate so we can use strtol/strtod. */ + saved = *value_endp; + *value_endp = '\0'; + + /* Integer, float, or neither? */ + errno = 0; + intval = strtol(value, &endptr, 0); + if (errno == 0 && *endptr == '\0' && endptr != value && + intval == (int) intval) + type = T_Integer; + else + { + type = T_Float; + (void) strtod(value, &endptr); + if (*endptr != '\0') + { + *value_endp = saved; + *errmsg = "invalid numeric value"; + return -1; + } + } + + /* Remove temporary terminator. */ + *value_endp = saved; + } + else + { + /* Identifier, possibly double-quoted. */ + type = T_String; + value = scan_identifier(&value_endp, &nextp, ',', true); + if (value == NULL) + { + /* + * scan_identifier will return NULL if it finds an + * unterminated double-quoted identifier or it finds no + * identifier at all because the next character is + * whitespace or the separator character, here a comma. + * But the latter case is impossible here because the code + * above has skipped whitespace and checked for commas. + */ + *errmsg = "unterminated double-quoted string"; + return -1; + } + } + } + + /* Skip trailing whitespace. */ + while (scanner_isspace(*nextp)) + nextp++; + + /* Expect comma or end of string. */ + if (*nextp == ',') + { + nextp++; + while (scanner_isspace(*nextp)) + nextp++; + if (*nextp == '\0') + { + *errmsg = "trailing comma in option list"; + return -1; + } + } + else if (*nextp == '\0') + done = true; + else + { + *errmsg = "expected comma or end of option list"; + return -1; + } + + /* + * Now safe to null-terminate the name and value. We couldn't do this + * earlier because in the unquoted case, the null terminator position + * may coincide with a character that the scanning logic above still + * needed to read. + */ + *name_endp = '\0'; + if (value_endp != NULL) + *value_endp = '\0'; + + /* Always count this option, and store the details if there is room. */ + if (noptions < maxoptions) + { + options[noptions].name = name; + options[noptions].type = type; + options[noptions].value = value; + } + noptions++; + } + + return noptions; +} diff --git a/contrib/auto_explain/expected/extension_options.out b/contrib/auto_explain/expected/extension_options.out new file mode 100644 index 00000000000..b5a66772311 --- /dev/null +++ b/contrib/auto_explain/expected/extension_options.out @@ -0,0 +1,49 @@ +-- +-- Tests for auto_explain.log_extension_options. +-- +LOAD 'auto_explain'; +LOAD 'pg_overexplain'; +-- Various legal values with assorted quoting and whitespace choices. +SET auto_explain.log_extension_options = ''; +SET auto_explain.log_extension_options = 'debug, RANGE_TABLE'; +SET auto_explain.log_extension_options = 'debug TRUE '; +SET auto_explain.log_extension_options = ' debug 1,RAnge_table "off"'; +SET auto_explain.log_extension_options = $$"debug" tRuE, range_table 'false'$$; +-- Syntax errors. +SET auto_explain.log_extension_options = ','; +ERROR: invalid value for parameter "auto_explain.log_extension_options": "," +DETAIL: option name missing or empty +SET auto_explain.log_extension_options = ', range_table'; +ERROR: invalid value for parameter "auto_explain.log_extension_options": ", range_table" +DETAIL: option name missing or empty +SET auto_explain.log_extension_options = 'range_table, '; +ERROR: invalid value for parameter "auto_explain.log_extension_options": "range_table, " +DETAIL: trailing comma in option list +SET auto_explain.log_extension_options = 'range_table true false'; +ERROR: invalid value for parameter "auto_explain.log_extension_options": "range_table true false" +DETAIL: expected comma or end of option list +SET auto_explain.log_extension_options = '"range_table'; +ERROR: invalid value for parameter "auto_explain.log_extension_options": ""range_table" +DETAIL: option name missing or empty +SET auto_explain.log_extension_options = 'range_table 3.1415nine'; +ERROR: invalid value for parameter "auto_explain.log_extension_options": "range_table 3.1415nine" +DETAIL: invalid numeric value +SET auto_explain.log_extension_options = 'range_table "true'; +ERROR: invalid value for parameter "auto_explain.log_extension_options": "range_table "true" +DETAIL: unterminated double-quoted string +SET auto_explain.log_extension_options = $$range_table 'true$$; +ERROR: invalid value for parameter "auto_explain.log_extension_options": "range_table 'true" +DETAIL: unterminated single-quoted string +SET auto_explain.log_extension_options = $$'$$; +ERROR: unrecognized EXPLAIN option "'" +-- Unacceptable option values. +SET auto_explain.log_extension_options = 'range_table maybe'; +ERROR: EXPLAIN option "range_table" requires a Boolean value +SET auto_explain.log_extension_options = 'range_table 2'; +ERROR: EXPLAIN option "range_table" requires a Boolean value +SET auto_explain.log_extension_options = 'range_table "0"'; +ERROR: EXPLAIN option "range_table" requires a Boolean value +SET auto_explain.log_extension_options = 'range_table 3.14159'; +ERROR: EXPLAIN option "range_table" requires a Boolean value +-- Supply enough options to force the option array to be reallocated. +SET auto_explain.log_extension_options = 'debug, debug, debug, debug, debug, debug, debug, debug, debug, debug false'; diff --git a/contrib/auto_explain/meson.build b/contrib/auto_explain/meson.build index 6f9d22bf5d8..d2b0650af1c 100644 --- a/contrib/auto_explain/meson.build +++ b/contrib/auto_explain/meson.build @@ -23,6 +23,7 @@ tests += { 'regress': { 'sql': [ 'alter_reset', + 'extension_options', ], }, 'tap': { diff --git a/contrib/auto_explain/sql/extension_options.sql b/contrib/auto_explain/sql/extension_options.sql new file mode 100644 index 00000000000..98920e88c9f --- /dev/null +++ b/contrib/auto_explain/sql/extension_options.sql @@ -0,0 +1,33 @@ +-- +-- Tests for auto_explain.log_extension_options. +-- + +LOAD 'auto_explain'; +LOAD 'pg_overexplain'; + +-- Various legal values with assorted quoting and whitespace choices. +SET auto_explain.log_extension_options = ''; +SET auto_explain.log_extension_options = 'debug, RANGE_TABLE'; +SET auto_explain.log_extension_options = 'debug TRUE '; +SET auto_explain.log_extension_options = ' debug 1,RAnge_table "off"'; +SET auto_explain.log_extension_options = $$"debug" tRuE, range_table 'false'$$; + +-- Syntax errors. +SET auto_explain.log_extension_options = ','; +SET auto_explain.log_extension_options = ', range_table'; +SET auto_explain.log_extension_options = 'range_table, '; +SET auto_explain.log_extension_options = 'range_table true false'; +SET auto_explain.log_extension_options = '"range_table'; +SET auto_explain.log_extension_options = 'range_table 3.1415nine'; +SET auto_explain.log_extension_options = 'range_table "true'; +SET auto_explain.log_extension_options = $$range_table 'true$$; +SET auto_explain.log_extension_options = $$'$$; + +-- Unacceptable option values. +SET auto_explain.log_extension_options = 'range_table maybe'; +SET auto_explain.log_extension_options = 'range_table 2'; +SET auto_explain.log_extension_options = 'range_table "0"'; +SET auto_explain.log_extension_options = 'range_table 3.14159'; + +-- Supply enough options to force the option array to be reallocated. +SET auto_explain.log_extension_options = 'debug, debug, debug, debug, debug, debug, debug, debug, debug, debug false'; diff --git a/contrib/auto_explain/t/001_auto_explain.pl b/contrib/auto_explain/t/001_auto_explain.pl index 5f673bd14c1..b4e8e4b65a1 100644 --- a/contrib/auto_explain/t/001_auto_explain.pl +++ b/contrib/auto_explain/t/001_auto_explain.pl @@ -30,7 +30,7 @@ sub query_log my $node = PostgreSQL::Test::Cluster->new('main'); $node->init(auth_extra => [ '--create-role' => 'regress_user1' ]); $node->append_conf('postgresql.conf', - "session_preload_libraries = 'auto_explain'"); + "session_preload_libraries = 'pg_overexplain,auto_explain'"); $node->append_conf('postgresql.conf', "auto_explain.log_min_duration = 0"); $node->append_conf('postgresql.conf', "auto_explain.log_analyze = on"); $node->start; @@ -172,6 +172,22 @@ like( qr/"Node Type": "Index Scan"[^}]*"Index Name": "pg_class_relname_nsp_index"/s, "index scan logged, json mode"); +# Extension options. +$log_contents = query_log( + $node, + "SELECT 1;", + { "auto_explain.log_extension_options" => "debug" }); + +like( + $log_contents, + qr/Parallel Safe:/, + "extension option produces per-node output"); + +like( + $log_contents, + qr/Command Type: select/, + "extension option produces per-plan output"); + # Check that PGC_SUSET parameters can be set by non-superuser if granted, # otherwise not diff --git a/doc/src/sgml/auto-explain.sgml b/doc/src/sgml/auto-explain.sgml index 15c868021e6..ee85a67eb2e 100644 --- a/doc/src/sgml/auto-explain.sgml +++ b/doc/src/sgml/auto-explain.sgml @@ -245,6 +245,29 @@ LOAD 'auto_explain'; </listitem> </varlistentry> + <varlistentry id="auto-explain-configuration-parameters-log-extension-options"> + <term> + <varname>auto_explain.log_extension_options</varname> (<type>string</type>) + <indexterm> + <primary><varname>auto_explain.log_extension_options</varname> configuration parameter</primary> + </indexterm> + </term> + <listitem> + <para> + Loadable modules can extend the <literal>EXPLAIN</literal> command with + additional options that affect the output format. Such options can also + be specified here. The value of this parameter is a comma-separated + list of options, each of which is an option name followed optionally by + an associated value. The module that provides the + <literal>EXPLAIN</literal> option, such as + <link linkend="pgplanadvice"><literal>pg_plan_advice</literal></link> or + <link linkend="pgoverexplain"><literal>pg_overexplain</literal></link>, + should be loaded before this parameter is set. + Only superusers can change this setting. + </para> + </listitem> + </varlistentry> + <varlistentry id="auto-explain-configuration-parameters-log-level"> <term> <varname>auto_explain.log_level</varname> (<type>enum</type>) diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list index 5bc517602b1..db83e52ccc5 100644 --- a/src/tools/pgindent/typedefs.list +++ b/src/tools/pgindent/typedefs.list @@ -3566,6 +3566,8 @@ astreamer_verify astreamer_waldump astreamer_zstd_frame auth_password_hook_typ +auto_explain_extension_options +auto_explain_option autovac_table av_relation avc_cache -- 2.51.0 [application/octet-stream] v2-0002-Add-a-guc_check_handler-to-the-EXPLAIN-extension-.patch (9.8K, 3-v2-0002-Add-a-guc_check_handler-to-the-EXPLAIN-extension-.patch) download | inline diff: From cabc9e35cb17fecbb7680bf0c5c7d4f2c4478432 Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> 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 <[email protected]> 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 [application/octet-stream] v2-0001-Expose-helper-functions-scan_quoted_identifier-an.patch (8.5K, 4-v2-0001-Expose-helper-functions-scan_quoted_identifier-an.patch) download | inline diff: From bf1fe9300db95682e76dfc729c968d526e55a3be Mon Sep 17 00:00:00 2001 From: Robert Haas <[email protected]> Date: Mon, 30 Mar 2026 18:49:56 -0400 Subject: [PATCH v2 1/3] Expose helper functions scan_quoted_identifier and scan_identifier. Previously, this logic was embedded within SplitIdentifierString, SplitDirectoriesString, and SplitGUCList. Factoring it out saves a bit of duplicated code, and also makes it available to extensions that might want to do similar things without necessarily wanting to do exactly the same thing. Reviewed-by: Matheus Alcantara <[email protected]> Discussion: http://postgr.es/m/CA+Tgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w@mail.gmail.com --- src/backend/utils/adt/varlena.c | 189 +++++++++++++++++--------------- src/include/utils/varlena.h | 3 + 2 files changed, 102 insertions(+), 90 deletions(-) diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c index ecad6d62184..c0ff51bd2fc 100644 --- a/src/backend/utils/adt/varlena.c +++ b/src/backend/utils/adt/varlena.c @@ -2750,6 +2750,96 @@ textToQualifiedNameList(text *textval) return result; } +/* + * scan_quoted_identifier - In-place scanner for quoted identifiers. + * + * *nextp should point to the opening double-quote character, and will be + * updated to point just past the end. *endp is set to the position of + * the closing quote. The return value is the identifier, or NULL if the + * matching close-quote cannot be found. + * + * If we find two consecutive double quote characters, that doesn't end the + * identifier: instead, we collapse them into a double quote and include them + * in the resulting token. Note that this requires overwriting the rest of the + * string in place, including the portion beyond the final value of *nextp. + */ +char * +scan_quoted_identifier(char **endp, char **nextp) +{ + char *token = *nextp + 1; + + for (;;) + { + *endp = strchr(*nextp + 1, '"'); + if (*endp == NULL) + return NULL; /* mismatched quotes */ + if ((*endp)[1] != '"') + break; /* found end of quoted identifier */ + /* Collapse adjacent quotes into one quote, and look again */ + memmove(*endp, *endp + 1, strlen(*endp)); + *nextp = *endp; + } + /* *endp now points at the terminating quote */ + *nextp = *endp + 1; + + return token; +} + +/* + * scan_identifier - In-place scanner for quoted or unquoted identifiers. + * + * On success, *endp is set to the position where the caller should write '\0' + * to null-terminate the token, and *nextp is advanced past the token (and past + * the closing quote, if any). The return value is the token content, or NULL + * if there is a syntax error (mismatched quotes or empty unquoted token). + * + * Unquoted identifiers are terminated by whitespace or the first occurrence + * of the separator character. Additionally, if downcase_unquoted = true, + * unquoted identifiers are downcased in place. See scan_quoted_identifier for + * an additional way in which we modify the string in place. + */ +char * +scan_identifier(char **endp, char **nextp, char separator, bool downcase_unquoted) +{ + char *token; + + if (**nextp == '"') + return scan_quoted_identifier(endp, nextp); + + /* Unquoted identifier --- extends to separator or whitespace */ + token = *nextp; + + while (**nextp && **nextp != separator && !scanner_isspace(**nextp)) + (*nextp)++; + + if (*nextp == token) + return NULL; /* empty token */ + + *endp = *nextp; + + if (downcase_unquoted) + { + /* + * Downcase the identifier, using same code as main lexer does. + * + * XXX because we want to overwrite the input in-place, we cannot + * support a downcasing transformation that increases the string + * length. This is not a problem given the current implementation of + * downcase_truncate_identifier, but we'll probably have to do + * something about this someday. + */ + int len = *endp - token; + char *downname = downcase_truncate_identifier(token, len, false); + + Assert(strlen(downname) <= len); + strncpy(token, downname, len); /* strncpy is required here */ + pfree(downname); + } + + return token; +} + + /* * SplitIdentifierString --- parse a string containing identifiers * @@ -2794,53 +2884,9 @@ SplitIdentifierString(char *rawstring, char separator, char *curname; char *endp; - if (*nextp == '"') - { - /* Quoted name --- collapse quote-quote pairs, no downcasing */ - curname = nextp + 1; - for (;;) - { - endp = strchr(nextp + 1, '"'); - if (endp == NULL) - return false; /* mismatched quotes */ - if (endp[1] != '"') - break; /* found end of quoted name */ - /* Collapse adjacent quotes into one quote, and look again */ - memmove(endp, endp + 1, strlen(endp)); - nextp = endp; - } - /* endp now points at the terminating quote */ - nextp = endp + 1; - } - else - { - /* Unquoted name --- extends to separator or whitespace */ - char *downname; - int len; - - curname = nextp; - while (*nextp && *nextp != separator && - !scanner_isspace(*nextp)) - nextp++; - endp = nextp; - if (curname == nextp) - return false; /* empty unquoted name not allowed */ - - /* - * Downcase the identifier, using same code as main lexer does. - * - * XXX because we want to overwrite the input in-place, we cannot - * support a downcasing transformation that increases the string - * length. This is not a problem given the current implementation - * of downcase_truncate_identifier, but we'll probably have to do - * something about this someday. - */ - len = endp - curname; - downname = downcase_truncate_identifier(curname, len, false); - Assert(strlen(downname) <= len); - strncpy(curname, downname, len); /* strncpy is required here */ - pfree(downname); - } + curname = scan_identifier(&endp, &nextp, separator, true); + if (curname == NULL) + return false; /* mismatched quotes or empty name */ while (scanner_isspace(*nextp)) nextp++; /* skip trailing whitespace */ @@ -2924,20 +2970,9 @@ SplitDirectoriesString(char *rawstring, char separator, if (*nextp == '"') { /* Quoted name --- collapse quote-quote pairs */ - curname = nextp + 1; - for (;;) - { - endp = strchr(nextp + 1, '"'); - if (endp == NULL) - return false; /* mismatched quotes */ - if (endp[1] != '"') - break; /* found end of quoted name */ - /* Collapse adjacent quotes into one quote, and look again */ - memmove(endp, endp + 1, strlen(endp)); - nextp = endp; - } - /* endp now points at the terminating quote */ - nextp = endp + 1; + curname = scan_quoted_identifier(&endp, &nextp); + if (curname == NULL) + return false; /* mismatched quotes */ } else { @@ -3042,35 +3077,9 @@ SplitGUCList(char *rawstring, char separator, char *curname; char *endp; - if (*nextp == '"') - { - /* Quoted name --- collapse quote-quote pairs */ - curname = nextp + 1; - for (;;) - { - endp = strchr(nextp + 1, '"'); - if (endp == NULL) - return false; /* mismatched quotes */ - if (endp[1] != '"') - break; /* found end of quoted name */ - /* Collapse adjacent quotes into one quote, and look again */ - memmove(endp, endp + 1, strlen(endp)); - nextp = endp; - } - /* endp now points at the terminating quote */ - nextp = endp + 1; - } - else - { - /* Unquoted name --- extends to separator or whitespace */ - curname = nextp; - while (*nextp && *nextp != separator && - !scanner_isspace(*nextp)) - nextp++; - endp = nextp; - if (curname == nextp) - return false; /* empty unquoted name not allowed */ - } + curname = scan_identifier(&endp, &nextp, separator, false); + if (curname == NULL) + return false; /* mismatched quotes or empty name */ while (scanner_isspace(*nextp)) nextp++; /* skip trailing whitespace */ diff --git a/src/include/utils/varlena.h b/src/include/utils/varlena.h index 4b32574a075..fe8d8a58952 100644 --- a/src/include/utils/varlena.h +++ b/src/include/utils/varlena.h @@ -27,6 +27,9 @@ extern int varstr_levenshtein_less_equal(const char *source, int slen, int ins_c, int del_c, int sub_c, int max_d, bool trusted); extern List *textToQualifiedNameList(text *textval); +extern char *scan_quoted_identifier(char **endp, char **nextp); +extern char *scan_identifier(char **endp, char **nextp, char separator, + bool downcase_unquoted); extern bool SplitIdentifierString(char *rawstring, char separator, List **namelist); extern bool SplitDirectoriesString(char *rawstring, char separator, -- 2.51.0 ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-02 21:41 Matheus Alcantara <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Matheus Alcantara @ 2026-04-02 21:41 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: [email protected]; Lukas Fittl <[email protected]>; Tom Lane <[email protected]> On Thu Apr 2, 2026 at 3:17 PM -03, Robert Haas wrote: > On Tue, Mar 31, 2026 at 6:10 PM Matheus Alcantara > <[email protected]> wrote: >> I think that we are safe against overflow because on >> auto_explain_split_options() it has intval == (int) intval, but I'm >> wondering if it's worth documenting this? > > We could add a comment here that the validity checks have already been > done at an earlier stage, but I felt like that was overkill. Possibly > not? > I think that it's useful but reading the new version I think that with a bit of debugging we can find that auto_explain_split_options() is the only function that set auto_explain_option->value and we check it for overflow before assigning. So yeah, I don't think that is mandatory for now. >> extension_options is being added to REGRESS in both Makefile and >> meson.build, but the actual test files are not included. > > Well, that sucks. I accidentally erased that file instead of > committing it. Here's a new version with mostly the same tests, plus I > updated the TAP test with a related test case as well. > +SET auto_explain.log_extension_options = $$'$$; +ERROR: unrecognized EXPLAIN option "'" IICU this is testing syntax errors and although we have the same error message when setting the GUC to an option that doesn't exists (e.g SET auto_explain.log_extension_options = 'wrong') I'm wondering if it should have a test case for this scenario, to ensure the behaviour? What do you think? >> + an associated value. The module that provides the >> + <literal>EXPLAIN</literal> option, such as >> + <link linkend="pgplanadvice"><literal>pg_plan_advice</literal></link> or >> + <link linkend="pgoverexplain"><literal>pg_overexplain</literal></link>, >> + should be loaded before this parameter is set. >> >> Wondering if this is clear enough about the shared_preload_libraries >> order (auto_explain should be loaded after extensions that include >> explain options) or if we should mention this explicitly. > > I actually thought that this might not be true until I tested it and > found that it sort of is. If you don't set > auto_explain.log_extension_options in postgresql.conf, then you can > load the modules in either order and it's fine. But if you do set it, > then you need to have the EXPLAIN option provider before auto_explain, > or else you get something like this: > > 2026-04-02 14:03:19.282 EDT [4614] WARNING: unrecognized EXPLAIN option "debug" > > ...because we read the whole postgresql.conf file before applying any > of it, and so if auto_explain's _PG_init() runs first, the GUC value > is already visible but the EXPLAIN option doesn't exist yet. That's > annoying, but I'm not sure it's worth any more of a documentation > reference than what I have already. "Before this parameter is set" > could be read to encompass "put it earlier in > shared_preload_libraries," and if someone does it wrong, it will > become obvious quickly enough. If you (or someone else) doesn't agree, > feel free to propose better wording -- I just don't want to expand the > description so much that it becomes a distraction. Yeah, make sense, agree. -- Matheus Alcantara EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-03 02:32 Lukas Fittl <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Lukas Fittl @ 2026-04-03 02:32 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; [email protected]; Tom Lane <[email protected]> On Tue, Mar 31, 2026 at 9:19 AM Robert Haas <[email protected]> wrote: > > On Mon, Mar 30, 2026 at 5:49 PM Robert Haas <[email protected]> wrote: > > I'm currently poking at some ideas for fixing this... more soon. > > Here are some patches. I got started poking at this in earnest > because, on the pg_plan_advice thread, Lukas was saying that instead > of adopting pg_collect_advice, we should just add an option to send > advice strings for each executed query to the server log. I went to > implement that and then felt like it should really be part of > auto_explain rather than its own thing, which took me down a bit of a > rathole. But I eventually found my way back out of it, so here's a > patch set implementing auto_explain.log_extension_options. Thanks for the effort, I think this is a good approach to solve the immediate need for plan advice (to have a facility to capture the advice strings for a set of queries), whilst avoiding introducing a new module, or completely new log settings. FWIW, initially I wasn't sure about this approach, since I typically see auto_explain focused on capturing outliers, so it wouldn't necessarily help you to capture the "good plan" (since that won't be an outlier). But since a superuser can modify auto_explain on a per-session basis this is similar to having a dedicated log setting, and it seems reasonable to not just have the advice string but also the plan that it is associated with. Its also useful that auto_explain has a sample rate option, so one could sample 1% of all queries for a few minutes to get a sense for the workload and the associated plan advice strings. And, just as a data point on why this is more generally useful besides pg_plan_advice and pg_overexplain: I've been thinking of utilizing the custom EXPLAIN option mechanism to log Plan ID values in EXPLAIN for an extension I maintain (pg_stat_plans), and this would allow that extension to also log the Plan IDs in auto_explain output, which would be very useful. > Anyway, if you apply all these patches it does solve the problem that > pg_collect_advice was targeting, modulo the need for some log parsing. > You can do this: > > pg_plan_advice.always_store_advice_details = on > auto_explain.log_min_duration = 0 > auto_explain.log_extension_options = 'plan_advice' > > And then you get log output like this: > > 2026-03-31 12:16:18.784 EDT [75224] LOG: duration: 0.013 ms plan: > Query Text: select 1; > Result (cost=0.00..0.01 rows=1 width=4) > Generated Plan Advice: > NO_GATHER("*RESULT*") Looks good, that makes sense to me in terms of user experience to target for this release. I have only skimmed the code before running out of energy for today, but will do a closer code review tomorrow. Thanks, Lukas -- Lukas Fittl ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-04 02:09 Lukas Fittl <[email protected]> parent: Robert Haas <[email protected]> 1 sibling, 5 replies; 16+ messages in thread From: Lukas Fittl @ 2026-04-04 02:09 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; [email protected]; Tom Lane <[email protected]> On Thu, Apr 2, 2026 at 11:17 AM Robert Haas <[email protected]> wrote: > > + an associated value. The module that provides the > > + <literal>EXPLAIN</literal> option, such as > > + <link linkend="pgplanadvice"><literal>pg_plan_advice</literal></link> or > > + <link linkend="pgoverexplain"><literal>pg_overexplain</literal></link>, > > + should be loaded before this parameter is set. > > > > Wondering if this is clear enough about the shared_preload_libraries > > order (auto_explain should be loaded after extensions that include > > explain options) or if we should mention this explicitly. > > I actually thought that this might not be true until I tested it and > found that it sort of is. If you don't set > auto_explain.log_extension_options in postgresql.conf, then you can > load the modules in either order and it's fine. But if you do set it, > then you need to have the EXPLAIN option provider before auto_explain, > or else you get something like this: > > 2026-04-02 14:03:19.282 EDT [4614] WARNING: unrecognized EXPLAIN option "debug" > > ...because we read the whole postgresql.conf file before applying any > of it, and so if auto_explain's _PG_init() runs first, the GUC value > is already visible but the EXPLAIN option doesn't exist yet. That's > annoying, but I'm not sure it's worth any more of a documentation > reference than what I have already. "Before this parameter is set" > could be read to encompass "put it earlier in > shared_preload_libraries," and if someone does it wrong, it will > become obvious quickly enough. If you (or someone else) doesn't agree, > feel free to propose better wording -- I just don't want to expand the > description so much that it becomes a distraction. Hmm. I think it would be typical to set "auto_explain.log_extension_options" in postgresql.conf, and this requirement of needing to put the libraries in the right order to avoid the warning doesn't seem great. As a general principle, I think we should aim to not have any ordering requirements in shared_preload_libraries. As one more data point, I know some cloud providers only expose "shared_preload_libraries" as a dropdown with options to check, i.e. you may not even have control over the order as an end user. And its unfortunate that "auto_explain" will often sort alphabetically before others.. Now the counter argument is that a WARNING is just that, i.e. it won't block the server from coming up. So maybe this is okay. I wonder if we should try to rework the code so that the GUC message when the option is not recognized is produced in auto_explain.c, and then add a GUC_check_errdetail that explains this may be due to the order of shared_preload_libraries. --- Now, onto the code: v2/0001 > +/* > + * scan_quoted_identifier - In-place scanner for quoted identifiers. > + * > + * *nextp should point to the opening double-quote character, and will be > + * updated to point just past the end. *endp is set to the position of > + * the closing quote. The return value is the identifier, or NULL if the > + * matching close-quote cannot be found. > + * Maybe make it clear in the comment that the return value is unterminated (e.g. "The return value is the unterminated identifier"), i.e. caller is responsible for setting the null byte based on endp. Otherwise 0001 looks good. v2/0002: > +bool > +GUCCheckExplainExtensionOption(const char *option_name, > + const char *option_value, > + NodeTag option_type) > +{ It feels a bit odd to not use an actual node here as the input argument (replacing option_value and option_type), or even pass a DefElem. But, if I followed your reasoning correctly, you're avoiding using DefElems here, because you don't want to keep nodes in auto_explain's GUC derived struct, to ensure we use guc_malloc for derived information. And presumably you're also modeling this particular method after GUCs in general, which don't have values represented as nodes. With that in mind, 0002 looks good as well. v2/0003: > +/* > + * GUC check hook for auto_explain.log_extension_options. > + */ > +static bool > +check_log_extension_options(char **newval, void **extra, GucSource source) > +{ > + char *rawstring; > + auto_explain_extension_options *result; > + auto_explain_option *options; > + int maxoptions = 8; > + Size rawstring_len; > + Size allocsize; > + char *errmsg; > + > + /* NULL or empty string means no options. */ > + if (*newval == NULL || (*newval)[0] == '\0') > + { > + *extra = NULL; Per src/backend/utils/misc/README, "extra is initialized to NULL before call, so it can be ignored if not needed." - so we don't have to set extra here. > +/* > + * Apply parsed extension options to an ExplainState. > + */ > +static void > +apply_extension_options(ExplainState *es, auto_explain_extension_options *ext) > +{ > + if (ext == NULL) > + return; > + > + for (int i = 0; i < ext->noptions; i++) > + { > + auto_explain_option *opt = &ext->options[i]; > + DefElem *def; > + Node *arg; > + > + if (opt->value == NULL) > + arg = NULL; > + else if (opt->type == T_Integer) > + arg = (Node *) makeInteger(strtol(opt->value, NULL, 0)); You note in a separate comment that we're using a subset of the main parser, but maybe its worth calling out integer values in particular, since things like "100_000" are not supported (since we're not using pg_strtoint64_safe + deal with the complexity of that in the scanning logic). I think this could be a code comment for now. Obviously we don't have a use case for that today, but thinking creatively, maybe someone wants to write an EXPLAIN extension that shows details for all nodes with cost values exceeding a large number, specified by an option. > +static int > +auto_explain_split_options(char *rawstring, auto_explain_option *options, > + int maxoptions, char **errmsg) > +{ I think this function probably has the most complexity due to its hand-rolled parsing, so it might be good if someone else takes another close look at it. FWIW, I couldn't find anything wrong from a first read. Otherwise 0003 looks good to me. Thanks, Lukas -- Lukas Fittl ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-04 02:39 Robert Haas <[email protected]> parent: Lukas Fittl <[email protected]> 4 siblings, 0 replies; 16+ messages in thread From: Robert Haas @ 2026-04-04 02:39 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; [email protected]; Tom Lane <[email protected]> On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <[email protected]> wrote: > Hmm. I think it would be typical to set > "auto_explain.log_extension_options" in postgresql.conf, and this > requirement of needing to put the libraries in the right order to > avoid the warning doesn't seem great. As a general principle, I think > we should aim to not have any ordering requirements in > shared_preload_libraries. I agree, but we're pretty far from that already today, and I don't see a way to avoid having this patch buy into it. And we're really, super-duper out of time at this point. We can throw this away, but we can't keep changing our minds. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 15:22 Robert Haas <[email protected]> parent: Lukas Fittl <[email protected]> 4 siblings, 0 replies; 16+ messages in thread From: Robert Haas @ 2026-04-06 15:22 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; [email protected]; Tom Lane <[email protected]> On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <[email protected]> wrote: > Maybe make it clear in the comment that the return value is > unterminated (e.g. "The return value is the unterminated identifier"), > i.e. caller is responsible for setting the null byte based on endp. > > Otherwise 0001 looks good. Thanks for the review, but I don't agree with that proposed change. To me, it seems too much like insisting that a function must document the things it doesn't do. Granted, saying "this function doesn't overwrite *endp" is a much more reasonable thing to document than "this function doesn't try to compute pi to 3,141,593 decimal places," because the latter is less likely to be something that the user would expect it to do. But I feel like the comment is clear enough as it is, so I've committed the patch without change. If more votes in favor of adjusting the comment emerge, we can certainly reconsider. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 16:46 Robert Haas <[email protected]> parent: Lukas Fittl <[email protected]> 4 siblings, 0 replies; 16+ messages in thread From: Robert Haas @ 2026-04-06 16:46 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; [email protected]; Tom Lane <[email protected]> On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <[email protected]> wrote: > v2/0002: > > > +bool > > +GUCCheckExplainExtensionOption(const char *option_name, > > + const char *option_value, > > + NodeTag option_type) > > +{ > > It feels a bit odd to not use an actual node here as the input > argument (replacing option_value and option_type), or even pass a > DefElem. > > But, if I followed your reasoning correctly, you're avoiding using > DefElems here, because you don't want to keep nodes in auto_explain's > GUC derived struct, to ensure we use guc_malloc for derived > information. And presumably you're also modeling this particular > method after GUCs in general, which don't have values represented as > nodes. Right, exactly. Although we sometimes cheat, a GUC's "extra" information is really supposed to consist of a single allocated chunk. I think that's a super-awkward constraint that we should really try to find a way to lift (and there was a thread about that earlier this release cycle which failed to reach a successful conclusion). But in this case, it seemed relatively straightforward to abide by that constraint, so I did. I'm actually pretty happy with the result: as we also discussed regarding pg_stash_advice and persistence, there's much to be said for fully validating the input first and only doing real work (such as allocating) afterward. I think I would like it better if the interface didn't need to refer to node types at all, but that seemed hard to avoid. What I'm more worried about with this patch is that the new infrastructure is too special-purpose. I think what I've learned here is that designing an interface around a single ExplainOptionHandler callback was a bad idea, because it had too specific an idea about how the callback was to be used and didn't leave future callers enough room to make their own decisions. This patch tries to paper around that mistake by adding ExplainOptionGUCCheckHandler, but I think there is a good chance that this will turn out to have exactly the same problem of being too specific to a particular use case. In other words, instead of fixing my earlier mistake, I'm making it a second time, which is usually not what you want to do. However, I still don't really know what I should be doing instead. If at some point in the future we figure out a way to separate EXPLAIN option validation and EXPLAIN option application in a cleaner way, I think we can refactor this for a future major release and just accept that extensions that provide EXPLAIN options will need updating. In the meantime, I think committing this is better than admitting defeat, so I have done that. > With that in mind, 0002 looks good as well. Thanks for looking! -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 19:27 Matheus Alcantara <[email protected]> parent: Lukas Fittl <[email protected]> 4 siblings, 2 replies; 16+ messages in thread From: Matheus Alcantara @ 2026-04-06 19:27 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; Robert Haas <[email protected]>; +Cc: [email protected]; Tom Lane <[email protected]> On 03/04/26 23:09, Lukas Fittl wrote: >> +static int >> +auto_explain_split_options(char *rawstring, auto_explain_option *options, >> + int maxoptions, char **errmsg) >> +{ > > I think this function probably has the most complexity due to its > hand-rolled parsing, so it might be good if someone else takes another > close look at it. > > FWIW, I couldn't find anything wrong from a first read. > > Otherwise 0003 looks good to me. > I did another review of 0003 (0001 and 0002 are now committed), focusing on auto_explain_split_options(). I didn't find any issues. The code coverage looks good - auto_explain_split_options() has nearly 100% coverage and the new code paths are well exercised by the tests. Regarding the shared_preload_libraries ordering concern: I don't think there's much we can do from the GUC perspective, I mean, we're essentially building an extension dependency chain when auto_explain.log_extension_options references options from other extensions. If I configure the GUC with 'plan_advice', I need to ensure pg_plan_advice is loaded before auto_explain. The documentation makes this clear. My concern is about that some cloud providers expose shared_preload_libraries as a dropdown without user control over ordering. I can be totally wrong, but it seems to me that in this case, the provider would need to handle dependencies appropriately or have a way to let the user define the ordering. Or, a possible improvement would be a post-configuration validation hook that runs after all shared_preload_libraries are loaded, allowing deferred validation of cross-extension dependencies like these EXPLAIN options (I'm wondering that we can have more extension dependencies in the future, e.g plan_advice and pg_stat_statements [1]) That said, I think we should proceed with 0003 as-is and revisit this when real-world usage reveals such problems in practice. [1] https://www.postgresql.org/message-id/CA%2BTgmobtbB74PTUtCQwUNR8D9ZZwKH5ZptoU30ONm34TR954Nw%40mail.g... -- Matheus Alcantara EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 19:30 Robert Haas <[email protected]> parent: Lukas Fittl <[email protected]> 4 siblings, 0 replies; 16+ messages in thread From: Robert Haas @ 2026-04-06 19:30 UTC (permalink / raw) To: Lukas Fittl <[email protected]>; +Cc: Matheus Alcantara <[email protected]>; [email protected]; Tom Lane <[email protected]> On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <[email protected]> wrote: > Per src/backend/utils/misc/README, "extra is initialized to NULL > before call, so it can be ignored if not needed." - so we don't have > to set extra here. Fair enough, but it's not an uncommon practice. See, e.g. check_synchronous_standby_names(), check_backtrace_functions(). > You note in a separate comment that we're using a subset of the main > parser, but maybe its worth calling out integer values in particular, > since things like "100_000" are not supported (since we're not using > pg_strtoint64_safe + deal with the complexity of that in the scanning > logic). I think this could be a code comment for now. OK, done. > I think this function probably has the most complexity due to its > hand-rolled parsing, so it might be good if someone else takes another > close look at it. Agreed, but I've committed this for now. If someone finds a problem later, it can be revised or reverted. -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 19:35 Robert Haas <[email protected]> parent: Matheus Alcantara <[email protected]> 1 sibling, 1 reply; 16+ messages in thread From: Robert Haas @ 2026-04-06 19:35 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Lukas Fittl <[email protected]>; [email protected]; Tom Lane <[email protected]> On Mon, Apr 6, 2026 at 3:27 PM Matheus Alcantara <[email protected]> wrote: > I did another review of 0003 (0001 and 0002 are now committed), focusing > on auto_explain_split_options(). I didn't find any issues. The code > coverage looks good - auto_explain_split_options() has nearly 100% > coverage and the new code paths are well exercised by the tests. Thanks. I just committed it. > My concern is about that some cloud providers expose > shared_preload_libraries as a dropdown without user control over > ordering. I can be totally wrong, but it seems to me that in this case, > the provider would need to handle dependencies appropriately or have a > way to let the user define the ordering. Or, a possible improvement > would be a post-configuration validation hook that runs after all > shared_preload_libraries are loaded, allowing deferred validation of > cross-extension dependencies like these EXPLAIN options (I'm wondering > that we can have more extension dependencies in the future, e.g > plan_advice and pg_stat_statements [1]) I think this probably collides rather badly with the GUC machinery: GUC validation can be deferred "a little bit," but the GUC system itself decides on the timing of validation, and there's no way for the GUC's check hook to say "please come back later". I suspect that property of the GUC system is too deeply embedded for us to think about changing it. > That said, I think we should proceed with 0003 as-is and revisit this > when real-world usage reveals such problems in practice. Yeah, it's frustrating to not be able to do something better than this for this release, but the great news is that there will very likely be another release next year. :-) -- Robert Haas EDB: http://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 19:50 Lukas Fittl <[email protected]> parent: Matheus Alcantara <[email protected]> 1 sibling, 0 replies; 16+ messages in thread From: Lukas Fittl @ 2026-04-06 19:50 UTC (permalink / raw) To: Matheus Alcantara <[email protected]>; +Cc: Robert Haas <[email protected]>; [email protected]; Tom Lane <[email protected]> On Mon, Apr 6, 2026 at 12:27 PM Matheus Alcantara <[email protected]> wrote: > My concern is about that some cloud providers expose > shared_preload_libraries as a dropdown without user control over > ordering. I can be totally wrong, but it seems to me that in this case, > the provider would need to handle dependencies appropriately or have a > way to let the user define the ordering. Or, a possible improvement > would be a post-configuration validation hook that runs after all > shared_preload_libraries are loaded, allowing deferred validation of > cross-extension dependencies like these EXPLAIN options (I'm wondering > that we can have more extension dependencies in the future, e.g > plan_advice and pg_stat_statements [1]) > > That said, I think we should proceed with 0003 as-is and revisit this > when real-world usage reveals such problems in practice. Agreed, I think we can lean on providers to do their part to make sure this works. FWIW, the main database-as-a-service provider I'm aware of that has a drop down for shared_preload_libraries today is Azure, and they do use alphabetic ordering in the UI. However I just checked, and the actual shared_preload_libraries value is not alphabetic (e.g. they put pg_cron first), so they just need to get the memo to handle this correctly. FWIW, AWS allows specifying the list in the same way postgresql.conf allows, and GCP has their own "cloudsql.enable_" flags mechanism, which presumably can also be taught to do the right thing. Thanks, Lukas -- Lukas Fittl ^ permalink raw reply [nested|flat] 16+ messages in thread
* Re: Add custom EXPLAIN options support to auto_explain @ 2026-04-06 19:58 Matheus Alcantara <[email protected]> parent: Robert Haas <[email protected]> 0 siblings, 0 replies; 16+ messages in thread From: Matheus Alcantara @ 2026-04-06 19:58 UTC (permalink / raw) To: Robert Haas <[email protected]>; +Cc: Lukas Fittl <[email protected]>; [email protected]; Tom Lane <[email protected]> On Mon Apr 6, 2026 at 4:35 PM -03, Robert Haas wrote: >> My concern is about that some cloud providers expose >> shared_preload_libraries as a dropdown without user control over >> ordering. I can be totally wrong, but it seems to me that in this case, >> the provider would need to handle dependencies appropriately or have a >> way to let the user define the ordering. Or, a possible improvement >> would be a post-configuration validation hook that runs after all >> shared_preload_libraries are loaded, allowing deferred validation of >> cross-extension dependencies like these EXPLAIN options (I'm wondering >> that we can have more extension dependencies in the future, e.g >> plan_advice and pg_stat_statements [1]) > > I think this probably collides rather badly with the GUC machinery: > GUC validation can be deferred "a little bit," but the GUC system > itself decides on the timing of validation, and there's no way for the > GUC's check hook to say "please come back later". I suspect that > property of the GUC system is too deeply embedded for us to think > about changing it. I was saying something like post_guc_init_hook() that is called after all GUC variables are initialized. For example, the check_log_extension_options() will only check if the syntax is correct and later the post_guc_init_hook() (via auto_explain) will be responsible to check that the options are valid or not. I didn't check the code in detail to see if this make any sense or not, but if it make I don't think that will be easy either because we call InitializeGUCOptions() very early than process_shared_preload_libraries(). Anyway this was something that I thought during the discussion. -- Matheus Alcantara EDB: https://www.enterprisedb.com ^ permalink raw reply [nested|flat] 16+ messages in thread
end of thread, other threads:[~2026-04-06 19:58 UTC | newest] Thread overview: 16+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-03-26 19:18 Add custom EXPLAIN options support to auto_explain Matheus Alcantara <[email protected]> 2026-03-30 21:49 ` Robert Haas <[email protected]> 2026-03-31 16:18 ` Robert Haas <[email protected]> 2026-03-31 22:10 ` Matheus Alcantara <[email protected]> 2026-04-02 18:17 ` Robert Haas <[email protected]> 2026-04-02 21:41 ` Matheus Alcantara <[email protected]> 2026-04-04 02:09 ` Lukas Fittl <[email protected]> 2026-04-04 02:39 ` Robert Haas <[email protected]> 2026-04-06 15:22 ` Robert Haas <[email protected]> 2026-04-06 16:46 ` Robert Haas <[email protected]> 2026-04-06 19:27 ` Matheus Alcantara <[email protected]> 2026-04-06 19:35 ` Robert Haas <[email protected]> 2026-04-06 19:58 ` Matheus Alcantara <[email protected]> 2026-04-06 19:50 ` Lukas Fittl <[email protected]> 2026-04-06 19:30 ` Robert Haas <[email protected]> 2026-04-03 02:32 ` Lukas Fittl <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox