On Mon, Jun 15, 2026 at 9:26 AM torikoshia <torikoshia@oss.nttdata.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:
+#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.
+ 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.
+++ 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.
+ * 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.
+extern TupleTableSlot *ExecProcNodeWithExplain(PlanState *ps);
There's no such function (any more, anyway).
--
Robert Haas
EDB: http://www.enterprisedb.com
