On Fri, Jan 20, 2023 at 12:32:58PM +0900, Michael Paquier wrote:
> FWIW, no objections from here. This maps with EXPLAIN where the query
> ID is only printed under VERBOSE.
While looking at this change, I have been wondering about something..
Isn't the knowledge of the query ID something that should be pushed
within ExplainPrintPlan() so as we don't duplicate in two places the
checks that show it? In short, the patch ignores the case where
compute_query_id = regress in auto_explain.
ExplainPrintTriggers() is kind of different because there is
auto_explain_log_triggers. Still, we could add a flag in ExplainState
deciding if the triggers should be printed, so as it would be possible
to move ExplainPrintTriggers() and ExplainPrintJITSummary() within
ExplainPrintPlan(), as well? The same kind of logic could be applied
for the planning time and the buffer usage if these are tracked in
ExplainState rather than being explicit arguments of ExplainOnePlan().
Not to mention that this reduces the differences between
ExplainOneUtility() and ExplainOnePlan().
Leaving this comment aside, I think that this should have at least one
regression test in 001_auto_explain.pl, where query_log() can be
called while the verbose GUC of auto_explain is enabled.
--
Michael