MIME-Version: 1.0
References: <DHCYFNC0WIF4.3T1M8AUK0DG7T@gmail.com>
In-Reply-To: <DHCYFNC0WIF4.3T1M8AUK0DG7T@gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 30 Mar 2026 17:49:55 -0400
Message-ID: 
 <CA+Tgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w@mail.gmail.com>
Subject: Re: Add custom EXPLAIN options support to auto_explain
To: Matheus Alcantara <matheusssilv97@gmail.com>
Cc: pgsql-hackers@lists.postgresql.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CA%2BTgmob-0W8306mvrJX5Urtqt1AAasu8pi4yLrZ1XfwZU-Uj1w%40mail.gmail.com>
Precedence: bulk

On Thu, Mar 26, 2026 at 3:18=E2=80=AFPM 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=3Dtrue 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.

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