Re: Record queryid when auto_explain.log_verbose is on

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