Horton

Thread

Commits

  1. auto_explain: Add new GUC, auto_explain.log_extension_options.

  2. Add a guc_check_handler to the EXPLAIN extension mechanism.

  3. Expose helper functions scan_quoted_identifier and scan_identifier.

  1. Add custom EXPLAIN options support to auto_explain

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-03-26T19:18:26Z

    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

  2. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-03-30T21:49:55Z

    On Thu, Mar 26, 2026 at 3:18 PM Matheus Alcantara
<matheusssilv97@gmail.com> 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

  3. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-03-31T16:18:48Z

    On Mon, Mar 30, 2026 at 5:49 PM Robert Haas <robertmhaas@gmail.com> 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

  4. Re: Add custom EXPLAIN options support to auto_explain

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-03-31T22:10:08Z

    On Tue Mar 31, 2026 at 1:18 PM -03, Robert Haas wrote:
> On Mon, Mar 30, 2026 at 5:49 PM Robert Haas <robertmhaas@gmail.com> 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

  5. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-04-02T18:17:20Z

    On Tue, Mar 31, 2026 at 6:10 PM Matheus Alcantara
<matheusssilv97@gmail.com> 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

  6. Re: Add custom EXPLAIN options support to auto_explain

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-04-02T21:41:35Z

    On Thu Apr 2, 2026 at 3:17 PM -03, Robert Haas wrote:
> On Tue, Mar 31, 2026 at 6:10 PM Matheus Alcantara
> <matheusssilv97@gmail.com> 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

  7. Re: Add custom EXPLAIN options support to auto_explain

    Lukas Fittl <lukas@fittl.com> — 2026-04-03T02:32:36Z

    On Tue, Mar 31, 2026 at 9:19 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Mon, Mar 30, 2026 at 5:49 PM Robert Haas <robertmhaas@gmail.com> 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

  8. Re: Add custom EXPLAIN options support to auto_explain

    Lukas Fittl <lukas@fittl.com> — 2026-04-04T02:09:00Z

    On Thu, Apr 2, 2026 at 11:17 AM Robert Haas <robertmhaas@gmail.com> 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

  9. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-04-04T02:39:10Z

    On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <lukas@fittl.com> 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

  10. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-04-06T15:22:42Z

    On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <lukas@fittl.com> 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

  11. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-04-06T16:46:45Z

    On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <lukas@fittl.com> 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

  12. Re: Add custom EXPLAIN options support to auto_explain

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-04-06T19:27:37Z

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

--
Matheus Alcantara
EDB: https://www.enterprisedb.com

  13. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-04-06T19:30:49Z

    On Fri, Apr 3, 2026 at 10:09 PM Lukas Fittl <lukas@fittl.com> 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

  14. Re: Add custom EXPLAIN options support to auto_explain

    Robert Haas <robertmhaas@gmail.com> — 2026-04-06T19:35:00Z

    On Mon, Apr 6, 2026 at 3:27 PM Matheus Alcantara
<matheusssilv97@gmail.com> 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

  15. Re: Add custom EXPLAIN options support to auto_explain

    Lukas Fittl <lukas@fittl.com> — 2026-04-06T19:50:59Z

    On Mon, Apr 6, 2026 at 12:27 PM Matheus Alcantara
<matheusssilv97@gmail.com> 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

  16. Re: Add custom EXPLAIN options support to auto_explain

    Matheus Alcantara <matheusssilv97@gmail.com> — 2026-04-06T19:58:01Z

    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