Subject: Add custom EXPLAIN options support to auto_explain

From: "Matheus Alcantara" <[email-masked]>
Date: Thu, 26 Mar 2026 16:18:26 -0300

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-masked]>
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

---

From: Robert Haas <[email-masked]>
Date: Mon, 30 Mar 2026 17:49:55 -0400

> (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.

> 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

---

From: Robert Haas <[email-masked]>
Date: Tue, 31 Mar 2026 12:18:48 -0400

> 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?

---

From: "Matheus Alcantara" <[email-masked]>
Date: Tue, 31 Mar 2026 19:10:08 -0300

On Tue Mar 31, 2026 at 1:18 PM -03, Robert Haas wrote:
> ...
> ...

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.

---

From: Robert Haas <[email-masked]>
Date: Thu, 2 Apr 2026 14:17:20 -0400

> 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?

> 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.

> 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.

---

From: "Matheus Alcantara" <[email-masked]>
Date: Thu, 02 Apr 2026 18:41:35 -0300

> done at an earlier stage, but I felt like that was overkill. Possibly

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.

> 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?

> description so much that it becomes a distraction.

Yeah, make sense, agree.

---

From: Lukas Fittl <[email-masked]>
Date: Thu, 2 Apr 2026 19:32:36 -0700

> 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.

> Result  (cost=0.00..0.01 rows=1 width=4)

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

---

From: Lukas Fittl <[email-masked]>
Date: Fri, 3 Apr 2026 19:09:00 -0700

> 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. */
> +        *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;

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

---

From: Robert Haas <[email-masked]>
Date: Fri, 3 Apr 2026 22:39:10 -0400

> 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.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 11:22:42 -0400

> 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.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 12:46:45 -0400

> method after GUCs in general, which don't have values represented as

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!

---

From: "Matheus Alcantara" <[email-masked]>
Date: Mon, 06 Apr 2026 16:27:37 -0300

> 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.gmail.com

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 15:30:49 -0400

> before call, so it can be ignored if not needed." - so we don't have

Fair enough, but it's not an uncommon practice. See, e.g.
check_synchronous_standby_names(), check_backtrace_functions().

> logic). I think this could be a code comment for now.

OK, done.

> hand-rolled parsing, so it might be good if someone else takes another

Agreed, but I've committed this for now. If someone finds a problem
later, it can be revised or reverted.

---

From: Robert Haas <[email-masked]>
Date: Mon, 6 Apr 2026 15:35:00 -0400

> coverage and the new code paths are well exercised by the tests.

Thanks. I just committed it.

> 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.

> 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. :-)

---

From: Lukas Fittl <[email-masked]>
Date: Mon, 6 Apr 2026 12:50:59 -0700

> 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

---

From: "Matheus Alcantara" <[email-masked]>
Date: Mon, 06 Apr 2026 16:58:01 -0300

> property of the GUC system is too deeply embedded for us to think

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.