On 2025-04-02 03:52, Robert Haas wrote:
Thank you for review!
On Fri, Mar 21, 2025 at 8:40 AM torikoshia <torikos...@oss.nttdata.com>
wrote:
Rebased it again.
On 2025-03-10 14:10, torikoshia wrote:
> BTW the patch adds about 400 lines to explain.c and it may be better
> to split the file as well as 9173e8b6046, but I leave it as it is for
> now.
This part remains unchanged.
Looking at ExplainAssembleLogOutput() is making me realize that
auto_explain is in serious need of some cleanup. That's not really the
fault of this patch, but the hack whereby we overwrite the [] that
would have surrounded the JSON output with {} is not very nice. I also
think that the auto_explain GUCs need rethinking. In theory, new
EXPLAIN options should be mirrored into auto_explain, but if you
compare ExplainAssembleLogOutput() to ExplainOnePlan(), you can see
that they are diverging. The PLANNING, SUMMARY, and SERIALIZE options
that are known to regular EXPLAIN aren't known to auto_explain, and
any customizable options that use explain_per_plan_hook won't be able
to work with auto_explain, either. Changing this is non-trivial
because SERIALIZE, for example, can't work the same way for
auto_explain as it does for EXPLAIN, and a full solution might also
require user-visible changes like replacing
auto_explain.log_<whatever> with auto_explain.explain, so I don't
really know. Maybe we should just live with it the way it is for now,
but it doesn't look very nice.
Rafael and I were just discussing off-list to what extent the parallel
query problems with his patch also apply to yours. His design makes
the problem easier to hit, I think, because setting
progressive_explain = on makes every backend attempt to dump a query
plan, whereas you'd have to hit the worker process with a signal and
just the right time. But the fundamental problem appears to be the
same.
In this patch, plan output is restricted to processes with B_BACKEND.
When targeting parallel workers, the plan is not attempted to be output,
as shown below:
=# select pg_log_query_plan(pid) from pg_stat_activity where
backend_type = 'parallel worker';
pg_log_query_plan
-------------------
f
f
(2 rows)
WARNING: PID 22720 is not a PostgreSQL client backend process
WARNING: PID 22719 is not a PostgreSQL client backend process
Given that the goal of this thread is simply to output the plan, I think
this is sufficient. Users can view the plan by accessing their leader
process.
However, I do agree that retrieving this information is necessary for
Rafael's work on tracking execution progress.
I think tracking execution progress involves more challenges to solve
compared to simply outputting the plan.
For this reason, I believe an incremental approach -- first completing
the basic plan output functionality in this thread and then extending it
to support progress tracking -- would be the good way forward.
Does ActiveQueryDesc really need to be exposed to the whole system?
Could it be a file-level variable?
Until the latest version patch, my goal was to output the plan without
requiring prior configuration, and I haven't seen any other viable
approach.
However, for the next patch, I'm considering introducing a GUC to allow
prior setup before outputting the plan, in response to the previously
quoted comment:
One way in which this proposal seems safer than previous proposals is
that previous proposals have involved session A poking session B and
trying to get session B to emit an EXPLAIN on the fly with no prior
setup. That would be very useful, but I think it's more difficult and
more risky than this proposal, where all the configuration happens in
the session that is going to emit the EXPLAIN output.
With this change, it should be possible to use a file-level variable
instead.
I'm going to try to improve other points you raised.
--
Regards,
--
Atsushi Torikoshi
Seconded from NTT DATA GROUP CORPORATION to SRA OSS K.K.
