Re: expose parallel leader in CSV and log_line_prefix

[Michael Paquier]

On Mon, Jul 20, 2020 at 06:30:48PM -0500, Justin Pryzby wrote:
> This thread is about a new feature that I proposed which isn't yet committed
> (logging leader_pid).  But it raises a question which is immediately relevant
> to pg_stat_activity.leader_pid, which is committed for v13.  So feel free to
> move to a new thread or to the thread for commit b025f3.

For a change of this size, with everybody involved in the past
discussion already on this thread, and knowing that you already
created an open item pointing to this part of the thread, I am not
sure that I would bother spawning a new thread now :) 

> I see a couple options:
> 
> - Update the documentation only, saying something like "leader_pid: the lock
>   group leader.  For a process involved in parallel query, this is the 
> parallel
>   leader.  In particular, for the leader process itself, leader_pid = pid, and
>   it is not reset until the leader terminates (it does not change when 
> parallel
>   workers exit).  This leaves in place the "raw" view of the data structure,
>   which can be desirable, but can be perceived as exposing unfriendly
>   implementation details.
> 
> - Functional change to show leader_pid = NULL for the leader itself.  Maybe
>   the columns should only be not-NULL when st_backendType == B_BG_WORKER &&
>   bgw_type='parallel worker'.  Update documentation to say: "leader_pid: for
>   parallel workers, the PID of their leader process".  (not a raw view of the
>   "lock group leader").

Yeah, I don't mind revisiting that per the connection pooler argument.
And I'd rather keep the simple suggestion of upthread to leave the
field as NULL for the parallel group leader with a PID match but not a
backend type check so as this could be useful for other types of
processes.  This leads me to the attached with the docs updated
(tested with read-only pgbench spawning parallel workers with
pg_stat_activity queried in parallel), to be applied down to 13.
Thoughts are welcome.
--
Michael

diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 2aff739466..95738a4e34 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -735,7 +735,13 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
 				wait_event = pgstat_get_wait_event(raw_wait_event);
 
 				leader = proc->lockGroupLeader;
-				if (leader)
+
+				/*
+				 * Show the leader only for active parallel workers.  This
+				 * leaves the field as NULL for the leader of a parallel
+				 * group.
+				 */
+				if (leader && leader->pid != beentry->st_procpid)
 				{
 					values[29] = Int32GetDatum(leader->pid);
 					nulls[29] = false;
diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml
index dc49177c78..15c598b2a5 100644
--- a/doc/src/sgml/monitoring.sgml
+++ b/doc/src/sgml/monitoring.sgml
@@ -687,12 +687,9 @@ postgres   27093  0.0  0.0  30096  2752 ?        Ss   11:34   0:00 postgres: ser
        <structfield>leader_pid</structfield> <type>integer</type>
       </para>
       <para>
-       Process ID of the parallel group leader if this process is or
-       has been involved in parallel query, or null. This field is set
-       when a process wants to cooperate with parallel workers, and
-       remains set as long as the process exists. For a parallel group leader,
-       this field is set to its own process ID. For a parallel worker,
-       this field is set to the process ID of the parallel group leader.
+       Process ID of the parallel group leader if this process is involved
+       in parallel query, or null.  For a parallel group leader, this field
+       is <literal>NULL</literal>.
       </para></entry>
      </row>
 

signature.asc
Description: PGP signature