MIME-Version: 1.0
References: <DHCYFNC0WIF4.3T1M8AUK0DG7T@gmail.com>
 <CA+Tgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w@mail.gmail.com>
 <CA+TgmoYUdeCdGfk8H6Ni2obXVixLvYaDkRGtxKLEmaCVNffsVA@mail.gmail.com>
 <DHHB7U2PSFYG.1A0WVPTX5LNOD@gmail.com>
 <CA+TgmoacOujCREMtQwURTSokB+ks-eLgaQ5vBMdWG959XzFcrA@mail.gmail.com>
In-Reply-To: 
 <CA+TgmoacOujCREMtQwURTSokB+ks-eLgaQ5vBMdWG959XzFcrA@mail.gmail.com>
From: Lukas Fittl <lukas@fittl.com>
Date: Fri, 3 Apr 2026 19:09:00 -0700
Message-ID: 
 <CAP53PkwsnhmoLXJaUMKWc76S21GzYOX06qKyZdVZhGGW0rCWKw@mail.gmail.com>
Subject: Re: Add custom EXPLAIN options support to auto_explain
To: Robert Haas <robertmhaas@gmail.com>
Cc: Matheus Alcantara <matheusssilv97@gmail.com>,
 pgsql-hackers@lists.postgresql.org,
	Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAP53PkwsnhmoLXJaUMKWc76S21GzYOX06qKyZdVZhGGW0rCWKw%40mail.gmail.com>
Precedence: bulk

On Thu, Apr 2, 2026 at 11:17=E2=80=AFAM Robert Haas <robertmhaas@gmail.com>=
 wrote:
> > +      an associated value. The module that provides the
> > +      <literal>EXPLAIN</literal> option, such as
> > +      <link linkend=3D"pgplanadvice"><literal>pg_plan_advice</literal>=
</link> or
> > +      <link linkend=3D"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 b=
e
> + * 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 sourc=
e)
> +{
> +    char       *rawstring;
> +    auto_explain_extension_options *result;
> +    auto_explain_option *options;
> +    int            maxoptions =3D 8;
> +    Size        rawstring_len;
> +    Size        allocsize;
> +    char       *errmsg;
> +
> +    /* NULL or empty string means no options. */
> +    if (*newval =3D=3D NULL || (*newval)[0] =3D=3D '\0')
> +    {
> +        *extra =3D 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 =3D=3D NULL)
> +        return;
> +
> +    for (int i =3D 0; i < ext->noptions; i++)
> +    {
> +        auto_explain_option *opt =3D &ext->options[i];
> +        DefElem    *def;
> +        Node       *arg;
> +
> +        if (opt->value =3D=3D NULL)
> +            arg =3D NULL;
> +        else if (opt->type =3D=3D T_Integer)
> +            arg =3D (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

--=20
Lukas Fittl