On Tue, Jan 24, 2023 at 11:01:46PM +0900, torikoshia wrote:
> On 2023-01-23 09:35, Michael Paquier wrote:
>> 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?
It could be. That's fine by me to not do that as a first step as the
query ID computation is done just after ExplainPrintPlan(). An
argument could be made about ExplainPrintPlan(), though
compute_query_id = regress offers an option to control that, as well.
>> 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.
Thanks. Will check and probably apply on HEAD.
--
Michael