Date: Fri, 10 Jul 2020 17:13:26 +0200
From: Julien Rouhaud <rjuju123@gmail.com>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	Daniel Gustafsson <daniel@yesql.se>, pgsql-hackers@postgresql.org
Subject: Re: expose parallel leader in CSV and log_line_prefix
Message-ID: <20200710151326.fics6czbbvloplsk@nol>
References: <20200315111831.GA21492@telsasoft.com>
 <20200315114933.udjdx5wcgwkjnw45@nol>
 <20200318212511.GN26184@telsasoft.com>
 <8EC64544-2EAA-48B1-8B69-85158CA3EAE9@yesql.se>
 <CAOBaU_a4=GXdqtXsU-2_o3oekEx3fsbCu-8ZB21oJm+CtfxNkA@mail.gmail.com>
 <20200710020940.GJ11153@paquier.xyz>
 <20200710022023.GE26220@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200710022023.GE26220@telsasoft.com>
Precedence: bulk

On Thu, Jul 09, 2020 at 09:20:23PM -0500, Justin Pryzby wrote:
> On Fri, Jul 10, 2020 at 11:09:40AM +0900, Michael Paquier wrote:
> > On Thu, Jul 09, 2020 at 01:53:39PM +0200, Julien Rouhaud wrote:
> > > Sure!  I've been quite busy with internal work duties recently but
> > > I'll review this patch shortly.  Thanks for the reminder!
> > 
> > Hmm.  In which cases would it be useful to have this information in
> > the logs knowing that pg_stat_activity lets us know the link between
> > both the leader and its workers?
> 
> PSA is an instantaneous view whereas the logs are a record.  That's important
> for shortlived processes (like background workers) or in the case of an ERROR
> or later crash.
> 
> Right now, the logs fail to include that information, which is deficient.  Half
> the utility is in showing *that* the log is for a parallel worker, which is
> otherwise not apparent.

Yes, I agree that this is a nice thing to have and another smell step toward
parallel query monitoring.

About the patch:

+           case 'k':
+               if (MyProc)
+               {
+                   PGPROC *leader = MyProc->lockGroupLeader;
+                   if (leader == NULL)
+                       /* padding only */
+                       appendStringInfoSpaces(buf,
+                               padding > 0 ? padding : -padding);
+                   else if (padding != 0)
+                       appendStringInfo(buf, "%*d", padding, leader->pid);
+                   else
+                       appendStringInfo(buf, "%d", leader->pid);
+               }
+               break;

There's a thinko in the padding handling.  It should be dones whether MyProc
and/or lockGroupLeader is NULL or not, and only if padding was asked, like it's
done for case 'd' for instance.

Also, the '%k' escape sounds a bit random.  Is there any reason why we don't
use any uppercase character for log_line_prefix?  %P could be a better
alternative, otherwise maybe %g, as GroupLeader/Gather?