> > FWIW, I have been thinking about auto_explain for another task,
> > remote plans for fdw [0], and perhaps there are now other good
> > reasons, some that you mention, that can be simplified if "auto_explain"
> > becomes a core feature. This could be a proposal taken up in 19.
>
> For what we're talking about here, I don't think we would need to go
> that far -- maybe put a few functions in core but no real need to move
> the whole module into core. However, I don't rule out that there are
> other reasons to do as you suggest.
This is the first core feature that will allow users to log explain plans for
a live workload. EXPLAIN is not really good for this purpose. So this
proposal is a step in the correct direction. I think the appetite for more
plan logging/visibility options will likely increase, and auto_explainlike
features in core will be desired IMO. We will see.
As far as this patch goes, I took a look and I have some comments:
1/
The name of ExplainAssembleLogOutput is not appropriate as
it does not really do anything with logs. Maybe ExplainStringAssemble
is more appropriate?
2/
It should be noted that the plan will not print to the log until
the plan begins executing the next plan node? depending on the
operation, that could take some time ( i.e.long seq scan of a table, etc.)
Does this behavior need to be called out in docs?
3/
+ * 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
Only superuser allowed to do this is very restrictive. Many shops do
not, for good
reasons, want DBAs or monitoring tools to connect as superuser. Why not allow
this functionality with "pg_monitor" ?
4/
nit: line removed here unnecessarily
extern PGDLLIMPORT Portal ActivePortal;
-
+extern PGDLLIMPORT QueryDesc *ActiveQueryDesc;
5/
WrapCustomPlanChildExecProcNodesWithExplain
This function name is too long, can the name be simplified?
6/
are such DEBUG1's really necessary, considering this is
a manually triggered function?
case T_Append:
ereport(DEBUG1, errmsg("wrapping Append"));
7/
Why do we only support TEXT format? I tried it with JSON
and it looks fine in the log as well. I can imagine automated
tools will want to be able to retrieve the plans using the
structured formats.
8/
+ es->format = EXPLAIN_FORMAT_TEXT;
+ es->settings = true;
+ es->verbose = true;
+ es->signaled = true;
+
+ ExplainAssembleLogOutput(es, ActiveQueryDesc,
EXPLAIN_FORMAT_TEXT, 0, -1);
+
Can we just pass es->format to ExplainAssembleLogOutput as the 3rd argument?
--
Sami Imseih
Amazon Web Services (AWS)
