On 2021/07/19 11:28, torikoshia wrote:
Agreed. Updated the patch.
Thanks for updating the patch! +bool +SendProcSignalForLogInfo(pid_t pid, ProcSignalReason reason) I don't think that procsignal.c is proper place to check the permission and check whether the specified PID indicates a PostgreSQL server process, etc because procsignal.c just provides fundamental routines for interprocess signaling. Isn't it better to move the function to signalfuncs.c or elsewhere? + ExplainQueryText(es, ActivePortal->queryDesc); + ExplainPrintPlan(es, ActivePortal->queryDesc); + ExplainPrintJITSummary(es, ActivePortal->queryDesc); When text format is used, ExplainBeginOutput() and ExplainEndOutput() do nothing. So (I guess) you thought that they don't need to be called and implemented the code in that way. But IMO it's better to comment why they don't need to be called, or to just call both of them even if they do nothing in text format. + ExplainPrintJITSummary(es, ActivePortal->queryDesc); It's better to check es->costs before calling this function, like explain_ExecutorEnd() and ExplainOnePlan() do? + result = SendProcSignalForLogInfo(pid, PROCSIG_LOG_CURRENT_PLAN); + + PG_RETURN_BOOL(result); Currently SendProcSignalForLogInfo() calls PG_RETURN_BOOL() in some cases, but instead it should just return true/false because pg_log_current_query_plan() expects that? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
