public inbox for [email protected]  

help / color / mirror / Atom feed
From: Fujii Masao <[email protected]>
To: torikoshia <[email protected]>
To: Masahiro Ikeda <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Dilip Kumar <[email protected]>
Cc: Pgsql Hackers <[email protected]>
Subject: Re: RFC: Logging plan of the running query
Date: Mon, 19 Jul 2021 15:07:48 +0900
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<CAFiTN-vQcDtyc5aRF1RNo-VgRzMcZ4E1qKTybvHpzUV1FEjqBA@mail.gmail.com>
	<CALj2ACWiVHpOqEztxHRtUR1oiDzPGUnguqv2EfEmQSv7jH93aw@mail.gmail.com>
	<CAFiTN-vOTFRK1xoAWRAGJ+54mSpDxmenAFexbySRhgy7hNH5Fg@mail.gmail.com>
	<CAFiTN-u6yqaxQyfX0jUMqDZ22fePfuQuQmzuozTxC34ca+ZByg@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CALj2ACUpznbsLbOg9mauSQZfU2eDxnNLYXcBHaAp1yWxMKLK9g@mail.gmail.com>
	<[email protected]>
	<CALj2ACVeOvBcN2-MZh-P3FKxsxOv-DD=fUgAMQEY8vPEhoJUQw@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<CALj2ACXS8oSHbO_gf==jRLGGDoMR-AoeKhoSfd1uh8kw-keY7g@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<[email protected]>



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





view thread (116+ messages)  latest in thread

reply
Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: RFC: Logging plan of the running query
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox