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