Date: Tue, 28 Jul 2020 10:10:33 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Justin Pryzby <pryzby@telsasoft.com>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>,
	Julien Rouhaud <rjuju123@gmail.com>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	pgsql-hackers@postgresql.org
Subject: Re: expose parallel leader in CSV and log_line_prefix
Message-ID: <20200728011033.GA28700@paquier.xyz>
References: <20200721043325.GC17300@paquier.xyz>
 <20200722153605.GA961@alvherre.pgsql>
 <20200723005214.GD26630@paquier.xyz>
 <20200726074206.GB4351@paquier.xyz>
 <20200726185427.GI4286@telsasoft.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="vtzGhvizbBRQ85DL"
Content-Disposition: inline
In-Reply-To: <20200726185427.GI4286@telsasoft.com>
Precedence: bulk


--vtzGhvizbBRQ85DL
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline

On Sun, Jul 26, 2020 at 01:54:27PM -0500, Justin Pryzby wrote:
> +            <row>
> +             <entry><literal>%P</literal></entry>
> +             <entry>For a parallel worker, this is the Process ID of its leader
> +             process.
> +             </entry>
> +             <entry>no</entry>
> +            </row>

Let's be a maximum simple and consistent with surrounding descriptions
here and what we have for pg_stat_activity, say:
"Process ID of the parallel group leader, if this process is a
parallel query worker."

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

It seems to me we should document here that the check on MyProcPid
ensures that this only prints the leader PID only for parallel workers
and discards the leader.

> +    appendStringInfoChar(&buf, ',');
> +
> +    /* leader PID */
> +    if (MyProc)
> +    {
> +        PGPROC *leader = MyProc->lockGroupLeader;
> +        if (leader && leader->pid != MyProcPid)
> +            appendStringInfo(&buf, "%d", leader->pid);
> +    }
> +

Same here.

Except for those nits, I have tested the patch and things behave as we
want (including padding and docs), so this looks good to me.
--
Michael

--vtzGhvizbBRQ85DL
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl8fewkACgkQnvQgOdby
QH3yCA/9EXezSrTB+m3g8I9zh8RkAqYmijQe84T9iA9BdhFltHlSp8RSSZf0xz2K
DRfSFfO5wmIPlrTfc++A2z7noHueruRRJa+yjwNyF0pCQr4iB1qwP1Pks/qeTrn9
l+AkAROmiNnMf2Jyvucf8qsZrbP/I96Wqvbo3kc4Qp8NMYNtK3Q/VQYxUPfVb4bo
47EiJYMe24GntehKwDRB6SeZldIgaazatKBoPC7HzaSg8M4XNZ23VTbrV73hyTA8
JhRJKQslo0HyzJvqn0jObO1ly2ZxFvLg/1A0hW9cAoLnkTN27ko9MMACxZ3mkcpC
n0HA1AzbSi7lI/QT5dOkqQS0au7x2nbPytOKPHK0K0Y9u5Aatz5g7sDwu8JCyw2M
DFP/xkQOobHiEC3q8qsh2woOQUj6PLLXQ+JrgFxz1VnegiSIV6qUEiUIDJjDwMNP
PhxUq574zbgF6kCrlp5/g30l8Wb/EUI9mhf0wHR60Ogv6AsYFyY1XzYuQdjzK9j5
7dJwpIvPqaD+POjcAIk+nXJwsULX/nA783mJ1y8loVtlit4HB9UWERh3n72jkJ9r
f3fCmb3/MKbNWEKgos1EDUVcN/IuuOn2DFuIeIUaQDmrBzdeU35QaNw/ZiEnPttm
JHuPNIA2uT5LskwKcHI90va2VMv4dKXxp0sPTEjADa8mL4NzPFI=
=xzox
-----END PGP SIGNATURE-----

--vtzGhvizbBRQ85DL--