Date: Fri, 12 Nov 2021 12:37:09 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Ekaterina Sokolova <e.sokolova@postgrespro.ru>
Cc: pgsql-hackers@lists.postgresql.org, torikoshia@oss.nttdata.com,
	Fujii Masao <masao.fujii@oss.nttdata.com>
Subject: Re: RFC: Logging plan of the running query
Message-ID: <20211112183709.GK17618@telsasoft.com>
References: <8b53b32f-26cc-0531-4ac0-27310e0bef4b@oss.nttdata.com>
 <CAFj8pRArY+1cGHrK1qOgbk-4auc27tbA338Ga6gh14LW8AtZpg@mail.gmail.com>
 <7958d03fffe3bafcfe8a505145c63928@oss.nttdata.com>
 <d4dffa658e5d52320eff88205c460b88@oss.nttdata.com>
 <9c4d9da6-48a5-fa18-4bff-f8eb0d4c9787@oss.nttdata.com>
 <59e6df324b41176d319f9c30bdb1d858@oss.nttdata.com>
 <0642712f-1298-960a-a3ba-e256d56040ac@oss.nttdata.com>
 <3d345c49561f31a1d8ff60ea86f88840@oss.nttdata.com>
 <c8b2b9f005e3d7ed51f6d70abfd641c4@oss.nttdata.com>
 <e0e7bf208bdeb2d988cdbb89fd9af905@postgrespro.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <e0e7bf208bdeb2d988cdbb89fd9af905@postgrespro.ru>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20211112183709.GK17618%40telsasoft.com>
Precedence: bulk

On Wed, Oct 13, 2021 at 05:28:30PM +0300, Ekaterina Sokolova wrote:
> Hi, hackers!
> 
>     • pg_query_state is contrib with 2 patches for core (I hope someday
> Community will support adding this patches to PostgreSQL). It contains

I reviewed this version of the patch - I have some language fixes.

I didn't know about pg_query_state, thanks.

> To improve this situation, this patch adds
> pg_log_current_query_plan() function that requests to log the
> plan of the specified backend process.

To me, "current plan" seems to mean "plan of *this* backend" (which makes no
sense to log).  I think the user-facing function could be called
pg_log_query_plan().  It's true that the implementation is a request to another
backend to log its *own* query plan - but users shouldn't need to know about
the implementation.

> +        Only superusers can request to log plan of the running query.

.. log the plan of a running query.

> +    Note that nested statements (statements executed inside a function) are not
> +    considered for logging. Only the deepest nesting query's plan is logged.

Only the plan of the most deeply nested query is logged.

> +				(errmsg("backend with PID %d is not running a query",
> +					MyProcPid)));

The extra parens around errmsg() are not needed since e3a87b499.

> +				(errmsg("backend with PID %d is now holding a page lock. Try again",

remove "now"

> +			(errmsg("plan of the query running on backend with PID %d is:\n%s",
> +					MyProcPid, es->str->data),

Maybe this should say "query plan running on backend with PID 17793 is:"

> + * would cause lots of log messages and which can lead to denial of

remove "and"

> +				 errmsg("must be a superuser to log information about specified process")));

I think it should say not say "specified", since that sounds like the user
might have access to log information about some other processes:
| must be a superuser to log information about processes

> +
> +	proc = BackendPidGetProc(pid);
> +
> +	/*
> +	 * BackendPidGetProc returns NULL if the pid isn't valid; but by the time
> +	 * we reach kill(), a process for which we get a valid proc here might
> +	 * have terminated on its own.  There's no way to acquire a lock on an
> +	 * arbitrary process to prevent that. But since this mechanism is usually
> +	 * used to below purposes, it might end its own first and the information

used for below purposes,

-- 
Justin