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