public inbox for [email protected]
help / color / mirror / Atom feedFrom: Robert Haas <[email protected]>
To: Lukas Fittl <[email protected]>
Cc: Matheus Alcantara <[email protected]>
Cc: [email protected]
Cc: Tom Lane <[email protected]>
Subject: Re: Add custom EXPLAIN options support to auto_explain
Date: Mon, 6 Apr 2026 12:46:45 -0400
Message-ID: <CA+TgmoZ=b6_34UeiOeQ3sz9WFAStZuiCa90pu6irHv=KsaDK8w@mail.gmail.com> (raw)
In-Reply-To: <CAP53PkwsnhmoLXJaUMKWc76S21GzYOX06qKyZdVZhGGW0rCWKw@mail.gmail.com>
References: <[email protected]>
<CA+Tgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w@mail.gmail.com>
<CA+TgmoYUdeCdGfk8H6Ni2obXVixLvYaDkRGtxKLEmaCVNffsVA@mail.gmail.com>
<[email protected]>
<CA+TgmoacOujCREMtQwURTSokB+ks-eLgaQ5vBMdWG959XzFcrA@mail.gmail.com>
<CAP53PkwsnhmoLXJaUMKWc76S21GzYOX06qKyZdVZhGGW0rCWKw@mail.gmail.com>
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
view thread (16+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Add custom EXPLAIN options support to auto_explain
In-Reply-To: <CA+TgmoZ=b6_34UeiOeQ3sz9WFAStZuiCa90pu6irHv=KsaDK8w@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox