Date: Fri, 10 Jul 2020 11:11:15 -0500
From: Justin Pryzby <pryzby@telsasoft.com>
To: Julien Rouhaud <rjuju123@gmail.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: <20200710161115.GK26220@telsasoft.com>
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>
 <20200710151326.fics6czbbvloplsk@nol>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="cWoXeonUoKmBZSoM"
Content-Disposition: inline
In-Reply-To: <20200710151326.fics6czbbvloplsk@nol>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk


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

On Fri, Jul 10, 2020 at 05:13:26PM +0200, Julien Rouhaud wrote:
> 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?

Thanks for looking.  %P is a good idea - it's consistent with ps and pkill and
probably other %commands.  I also amended the docs.

-- 
Justin

--cWoXeonUoKmBZSoM
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="v2-0001-Include-the-leader-PID-in-logfile.patch"

From 68a892d0023e915cc48aea1bb77eae5deabcecec Mon Sep 17 00:00:00 2001
From: Justin Pryzby <pryzbyj@telsasoft.com>
Date: Fri, 13 Mar 2020 22:03:06 -0500
Subject: [PATCH v2] Include the leader PID in logfile

See also: b025f32e0b, which adds the leader PID to pg_stat_activity
---
 doc/src/sgml/config.sgml                      | 11 ++++++-
 src/backend/utils/error/elog.c                | 30 +++++++++++++++++++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 3 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index b353c61683..e60403d6a5 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -6611,6 +6611,14 @@ local0.*    /var/log/postgresql
              <entry>Process ID</entry>
              <entry>no</entry>
             </row>
+            <row>
+             <entry><literal>%P</literal></entry>
+             <entry>Process ID of the parallel group leader if this process was
+             at some point involved in parallel query, otherwise null.  For a
+             parallel group leader itself, this field is set to its own process
+             ID.</entry>
+             <entry>no</entry>
+            </row>
             <row>
              <entry><literal>%t</literal></entry>
              <entry>Time stamp without milliseconds</entry>
@@ -6943,7 +6951,7 @@ log_line_prefix = '%m [%p] %q%u@%d/%a '
         character count of the error position therein,
         location of the error in the PostgreSQL source code
         (if <varname>log_error_verbosity</varname> is set to <literal>verbose</literal>),
-        application name, and backend type.
+        application name, backend type, and leader PID.
         Here is a sample table definition for storing CSV-format log output:
 
 <programlisting>
@@ -6973,6 +6981,7 @@ CREATE TABLE postgres_log
   location text,
   application_name text,
   backend_type text,
+  leader_pid integer,
   PRIMARY KEY (session_id, session_line_num)
 );
 </programlisting>
diff --git a/src/backend/utils/error/elog.c b/src/backend/utils/error/elog.c
index e4b717c79a..dea098faf1 100644
--- a/src/backend/utils/error/elog.c
+++ b/src/backend/utils/error/elog.c
@@ -77,6 +77,7 @@
 #include "postmaster/syslogger.h"
 #include "storage/ipc.h"
 #include "storage/proc.h"
+#include "storage/procarray.h"
 #include "tcop/tcopprot.h"
 #include "utils/guc.h"
 #include "utils/memutils.h"
@@ -2448,6 +2449,25 @@ log_line_prefix(StringInfo buf, ErrorData *edata)
 				else
 					appendStringInfo(buf, "%d", MyProcPid);
 				break;
+
+			case 'P':
+				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);
+				}
+				else if (padding != 0)
+					appendStringInfoSpaces(buf,
+										   padding > 0 ? padding : -padding);
+				break;
+
 			case 'l':
 				if (padding != 0)
 					appendStringInfo(buf, "%*ld", padding, log_line_number);
@@ -2836,6 +2856,16 @@ write_csvlog(ErrorData *edata)
 	else
 		appendCSVLiteral(&buf, GetBackendTypeDesc(MyBackendType));
 
+	appendStringInfoChar(&buf, ',');
+
+	/* leader PID */
+	if (MyProc)
+	{
+		PGPROC *leader = MyProc->lockGroupLeader;
+		if (leader)
+			appendStringInfo(&buf, "%d", leader->pid);
+	}
+
 	appendStringInfoChar(&buf, '\n');
 
 	/* If in the syslogger process, try to write messages direct to file */
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index e430e33c7b..48a50fb394 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -533,6 +533,7 @@
 					#   %h = remote host
 					#   %b = backend type
 					#   %p = process ID
+					#   %P = leader PID
 					#   %t = timestamp without milliseconds
 					#   %m = timestamp with milliseconds
 					#   %n = timestamp with milliseconds (as a Unix epoch)
-- 
2.17.0


--cWoXeonUoKmBZSoM--