Re: RFC: Logging plan of the running query - Mailing list pgsql-hackers

From Justin Pryzby
Subject Re: RFC: Logging plan of the running query
Date
Msg-id 20211112183709.GK17618@telsasoft.com
Whole thread Raw
In response to Re: RFC: Logging plan of the running query  (Ekaterina Sokolova <e.sokolova@postgrespro.ru>)
Responses Re: RFC: Logging plan of the running query
List pgsql-hackers
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



pgsql-hackers by date:

Previous
From: "Euler Taveira"
Date:
Subject: Re: Logical Replication - improve error message while adding tables to the publication in check_publication_add_relation
Next
From: Peter Geoghegan
Date:
Subject: Is heap_page_prune() stats collector accounting wrong?