Date: Mon, 20 Jul 2020 18:30:48 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: Julien Rouhaud <rjuju123@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	pgsql-hackers <pgsql-hackers@postgresql.org>
Subject: Re: expose parallel leader in CSV and log_line_prefix
Message-ID: <20200720233048.GC5748@telsasoft.com>
References: <20200717205421.GR23581@telsasoft.com>
 <20200717212721.GA3827@alvherre.pgsql>
 <20200717223236.GT23581@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200717223236.GT23581@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Fri, Jul 17, 2020 at 05:32:36PM -0500, Justin Pryzby wrote:
> On Fri, Jul 17, 2020 at 05:27:21PM -0400, Alvaro Herrera wrote:
> > On 2020-Jul-17, Justin Pryzby wrote:
> > > Ok, but should we then consider changing pg_stat_activity for consistency ?
> > > Probably in v13 to avoid changing it a year later.
> > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=b025f32e0b5d7668daec9bfa957edf3599f4baa8
> > > 
> > > I think the story is that we're exposing to the user a "leader pid" what's
> > > internally called (and used as) the "lock group leader", which for the leader
> > > process is set to its own PID.  But I think what we're exposing as leader_pid
> > > will seem like an implementation artifact to users.
> > 
> > IMO it *is* an implementation artifact if, as you say, the leader PID
> > remains set after the parallel query is done.  I mentioned the pgbouncer
> > case before: if you run a single parallel query, then the process
> > remains a "parallel leader" for days or weeks afterwards even if it
> > hasn't run a parallel query ever since.  That doesn't sound great to me.
> > 
> > I think it's understandable and OK if there's a small race condition
> > that means you report a process as a leader shortly before or shortly
> > after a parallel query is actually executed.  But doing so until backend
> > termination seems confusing as well as useless.
> 
> I'm not sure that connection pooling is the strongest argument against the
> current behavior, but we could change it as suggested to show as NULL the
> leader_pid for the leader's own process.  I think that's the intuitive behavior
> a user expects.  Parallel processes are those with leader_pid IS NOT NULL.  If
> we ever used lockGroupLeader for something else, you'd also have to say AND
> backend_type='parallel worker'.
> 
> We should talk about doing that for PSA and for v13 as well.  Here or on the
> other thread or a new thread ?  It's a simple enough change, but the question
> is if we want to provide a more "cooked" view whcih hides the internals, and if
> so, is this really enough.
> 
> --- a/src/backend/utils/adt/pgstatfuncs.c
> +++ b/src/backend/utils/adt/pgstatfuncs.c
> @@ -737,3 +737,4 @@ pg_stat_get_activity(PG_FUNCTION_ARGS)
>                                 leader = proc->lockGroupLeader;
> -                               if (leader)
> +                               if (leader && leader->pid != beentry->st_procpid)
>                                 {
>                                         values[29] = Int32GetDatum(leader->pid);

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.

I added this to the Opened Items list so it's not lost.

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").

- ??

-- 
Justin