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?