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>
 <CAP53PkwsnhmoLXJaUMKWc76S21GzYOX06qKyZdVZhGGW0rCWKw@mail.gmail.com>
In-Reply-To: 
 <CAP53PkwsnhmoLXJaUMKWc76S21GzYOX06qKyZdVZhGGW0rCWKw@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 6 Apr 2026 12:46:45 -0400
Message-ID: 
 <CA+TgmoZ=b6_34UeiOeQ3sz9WFAStZuiCa90pu6irHv=KsaDK8w@mail.gmail.com>
Subject: Re: Add custom EXPLAIN options support to auto_explain
To: Lukas Fittl <lukas@fittl.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/CA%2BTgmoZ%3Db6_34UeiOeQ3sz9WFAStZuiCa90pu6irHv%3DKsaDK8w%40mail.gmail.com>
Precedence: bulk

On Fri, Apr 3, 2026 at 10:09=E2=80=AFPM 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!

--=20
Robert Haas
EDB: http://www.enterprisedb.com