Re: RFC: Logging plan of the running query

On Mon, Jun 22, 2026 at 10:51 PM Robert Haas <robertmhaas(at)gmail(dot)com>
wrote:
>
> On Mon, Jun 15, 2026 at 9:26 AM torikoshia <torikoshia(at)oss(dot)nttdata(dot)com>
> wrote:
> > I also fixed some typos in the attached patch.
>
> In my earlier reviews, I have been focusing on core correctness
> issues. This time, I decided to leave that aside to focus on a few
> more cosmetic and user-interface points:

Thank you for the review!

> +#ifdef USE_INJECTION_POINTS
> + INJECTION_POINT("log-query-interrupt", NULL);
> +#endif
>
> I think the #ifdef is not required here. Look at how INJECTION_POINT()
> is defined.

Agreed.

> + es = NewExplainState();
> +
> + es->format = EXPLAIN_FORMAT_TEXT;
> + es->settings = true;
> + es->verbose = true;
> + es->signaled = true;
>
> So, we're establishing a non-default set of settings here which the
> user has no way to configure. I think that's hard to justify. I think
> we either need to accept the defaults that NewExplainState()
> configures, or we need to provide a GUC for the user to configure
> things as they wish.

At this stage, what I would like to focus on is developing the mechanism
for obtaining the plan of a currently running query. So I think it is
better to start with the default EXPLAIN options.
Updated the patch that way.

> +++ b/src/backend/commands/dynamic_explain.c
>
> We have four explain-related source files currently, all of them have
> "explain" at the start of the name rather than at the end. We should
> probably do the same here. I would suggest explain_running.c rather
> than explain_dynamic.c. Or some other word that conveys "the query is
> in progress" -- dynamic seems vague.

Thanks for the suggestion. Renamed it to explain_running.c.

> + * By default, only superusers are allowed to signal to log the plan
> because
> + * allowing any users to issue this request at an unbounded rate would
> + * cause lots of log messages and which can lead to denial of service.
>
> It's worth thinking about whether restricting this to the superuser by
> default is the right decision. I don't think I believe this rationale:
> a user can do lots of things that generate lots of log volume, and we
> don't restrict any of the other ones. For example, any user can do
> this:
>
> do $$ begin for i in 1..1000000 loop raise log '% is an extremely long
> string', repeat('hoge',100000); end loop; end; $$;
>
> I thought about whether we should actually relax this and let people
> log their own queries, but then I realized what I believe the real
> issue to be: the superuser is presumed to have access to the log files
> -- after all, they can configure them, and escape to the shell -- but
> other users very likely don't. If some of them do, the superuser knows
> which ones.

That makes sense.
Even if a non-superuser has EXECUTE permission on this function, it
would
probably not be very useful, because the user would not be able to read
the server log by themselves as you pointed out.

Would it make sense to restrict EXECUTE to superusers by default for
that
reason?
In the attached patch, I have rewritten the comment as follows:

+ * By default, only superusers are allowed to signal a backend to log
its
+ * plan because the output is written to the server log, which in
many cases
+ * can only be read by superusers.

The current restriction was modeled after
pg_log_backend_memory_contexts().
So I think the same discussion would also apply to that function. If
needed,
we could also revisit the comment there from the same perspective:

* pg_log_backend_memory_contexts
* Signal a backend or an auxiliary process to log its memory
contexts.
*
* By default, only superusers are allowed to signal to log the memory
* contexts because allowing any users to issue this request at an
unbounded
* rate would cause lots of log messages and which can lead to denial
of
* service. Additional roles can be permitted with GRANT.

BTW I'm interested in the proposal of
pg_get_process_memory_contexts()[1],
which would return memory context information directly from the function
rather than writing it to the log. If a similar approach were adopted
for running query plans in the future, then it would make sense to relax
the default privilege for that.

> +extern TupleTableSlot *ExecProcNodeWithExplain(PlanState *ps);
>
> There's no such function (any more, anyway).

Removed.

[1]
https://www.postgresql.org/message-id/495F9D76-6B8B-45C3-95DA-EF5A8762DA19%40yesql.se

Thanks,

--
Atsushi Torikoshi
Seconded from NTT DATA CORPORATION to SRA OSS K.K.