On 2023-01-23 09:35, Michael Paquier wrote:
> 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.
Thanks!
It seems better and updated the patch.
>
> 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().
Hmm, this refactoring would worth considering, but should be done in
another patch?
> 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.
Agreed.
Added a test for queryid logging.
> --
> Michael
--
Regards,
--
Atsushi Torikoshi
NTT DATA CORPORATION