Re: RFC: Logging plan of the running query

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