Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date: Thu, 02 Apr 2026 18:41:35 -0300
Message-Id: <DHIZV2B6QRO0.3PX0A6T2M0QB7@gmail.com>
To: "Robert Haas" <robertmhaas@gmail.com>
Cc: <pgsql-hackers@lists.postgresql.org>, "Lukas Fittl" <lukas@fittl.com>,
 "Tom Lane" <tgl@sss.pgh.pa.us>
Subject: Re: Add custom EXPLAIN options support to auto_explain
From: "Matheus Alcantara" <matheusssilv97@gmail.com>
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>
Archived-At: 
 <https://www.postgresql.org/message-id/DHIZV2B6QRO0.3PX0A6T2M0QB7%40gmail.com>
Precedence: bulk

On Thu Apr 2, 2026 at 3:17 PM -03, Robert Haas wrote:
> On Tue, Mar 31, 2026 at 6:10=E2=80=AFPM Matheus Alcantara
> <matheusssilv97@gmail.com> wrote:
>> I think that we are safe against overflow because on
>> auto_explain_split_options() it has intval =3D=3D (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 =3D $$'$$;
+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 =3D '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=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.

Yeah, make sense, agree.

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