Re: RFC: Logging plan of the running query - Mailing list pgsql-hackers

On 2025-04-02 03:52, Robert Haas wrote:
Thank you for review!
> On Fri, Mar 21, 2025 at 8:40 AM torikoshia <torikoshia@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.