Re: expose parallel leader in CSV and log_line_prefix

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?