[PATCH] Report the query string that caused a memory error under Valgrind

From 1d1aafdbe6b640ab66ec2fcf77852ba1bd7df084 Mon Sep 17 00:00:00 2001
From: Onur Tirtir <onurcantirtir@gmail.com>
Date: Thu, 23 Mar 2023 20:04:59 +0300
Subject: [PATCH v2] Report the query string that caused a memory error

If we run multiple queries under Valgrind, it becomes quite hard to
understand which query caused a memory error by looking into the stack
traces that Valgrind reported.

For this reason, this patch introduces some changes to
PostgresMain() to report the messages that cause memory errors. At
the high-level:
* We compare number of memory errors reported by Valgrind before and
  after this message-run by using VALGRIND_COUNT_ERRORS macro provided
  in valgrind.h.
* If the message caused a memory error, then we report this query to
  Valgrind log file (right after the relevant stack trace) by
  using VALGRIND_PRINTF macro provided in valgrind.h.
---
 src/backend/tcop/postgres.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index cab709b07b..aeb8ad34d7 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -73,6 +73,9 @@
 #include "utils/snapmgr.h"
 #include "utils/timeout.h"
 #include "utils/timestamp.h"
+#ifdef USE_VALGRIND
+#include <valgrind/valgrind.h>
+#endif
 
 /* ----------------
  *		global variables
@@ -4070,6 +4073,9 @@ PostgresMain(const char *dbname, const char *username)
 	volatile bool send_ready_for_query = true;
 	bool		idle_in_transaction_timeout_enabled = false;
 	bool		idle_session_timeout_enabled = false;
+#ifdef USE_VALGRIND
+	unsigned	valgrind_errors_before_message = 0;
+#endif
 
 	Assert(dbname != NULL);
 	Assert(username != NULL);
@@ -4365,6 +4371,10 @@ PostgresMain(const char *dbname, const char *username)
 
 	for (;;)
 	{
+#ifdef USE_VALGRIND
+		valgrind_errors_before_message = VALGRIND_COUNT_ERRORS;
+#endif
+
 		/*
 		 * At top of loop, reset extended-query-message flag, so that any
 		 * errors encountered in "idle" state don't provoke skip.
@@ -4802,6 +4812,22 @@ PostgresMain(const char *dbname, const char *username)
 						 errmsg("invalid frontend message type %d",
 								firstchar)));
 		}
+
+#ifdef USE_VALGRIND
+
+		/*
+		 * Report the message (and its type) that caused a memory error if
+		 * it's not something that resulted in a backend exit or an ereport()
+		 * call.
+		 */
+		if (VALGRIND_COUNT_ERRORS > valgrind_errors_before_message)
+		{
+			VALGRIND_PRINTF("The message for which valgrind reported a "
+							"memory error was (of type '%c'):\n%s\n\n",
+							firstchar,
+							input_message.len ? input_message.data : "(empty)");
+		}
+#endif
 	}							/* end of input-reading loop */
 }
 
-- 
2.25.1

diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index cab709b07b..a10ecbaf50 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -27,6 +27,10 @@
 #include <sys/socket.h>
 #include <sys/time.h>
 
+#ifdef USE_VALGRIND
+#include <valgrind/valgrind.h>
+#endif
+
 #include "access/parallel.h"
 #include "access/printtup.h"
 #include "access/xact.h"
@@ -191,6 +195,36 @@ static void enable_statement_timeout(void);
 static void disable_statement_timeout(void);
 
 
+/* ----------------------------------------------------------------
+ *		infrastructure for valgrind debugging
+ * ----------------------------------------------------------------
+ */
+#ifdef USE_VALGRIND
+/* This variable should be set at the top of the main loop. */
+static unsigned int old_valgrind_error_count;
+
+/*
+ * If Valgrind detected any errors since old_valgrind_error_count was updated,
+ * report the current query as the cause.  This should be called at the end
+ * of message processing.
+ */
+static void
+valgrind_report_error_query(const char *query)
+{
+	unsigned int valgrind_error_count = VALGRIND_COUNT_ERRORS;
+
+	if (unlikely(valgrind_error_count != old_valgrind_error_count) &&
+		query != NULL)
+		VALGRIND_PRINTF("Valgrind detected %u error(s) during execution of \"%s\"\n",
+						valgrind_error_count - old_valgrind_error_count,
+						query);
+}
+
+#else							/* !USE_VALGRIND */
+#define valgrind_report_error_query(query) ((void) 0)
+#endif							/* USE_VALGRIND */
+
+
 /* ----------------------------------------------------------------
  *		routines to obtain user input
  * ----------------------------------------------------------------
@@ -2041,6 +2075,8 @@ exec_bind_message(StringInfo input_message)
 	if (save_log_statement_stats)
 		ShowUsage("BIND MESSAGE STATISTICS");
 
+	valgrind_report_error_query(debug_query_string);
+
 	debug_query_string = NULL;
 }
 
@@ -2292,6 +2328,8 @@ exec_execute_message(const char *portal_name, long max_rows)
 	if (save_log_statement_stats)
 		ShowUsage("EXECUTE MESSAGE STATISTICS");
 
+	valgrind_report_error_query(debug_query_string);
+
 	debug_query_string = NULL;
 }
 
@@ -4287,6 +4325,12 @@ PostgresMain(const char *dbname, const char *username)
 		/* Report the error to the client and/or server log */
 		EmitErrorReport();
 
+		/*
+		 * If Valgrind noticed something during the erroneous query, print the
+		 * query string, assuming we have one.
+		 */
+		valgrind_report_error_query(debug_query_string);
+
 		/*
 		 * Make sure debug_query_string gets reset before we possibly clobber
 		 * the storage it points at.
@@ -4371,6 +4415,13 @@ PostgresMain(const char *dbname, const char *username)
 		 */
 		doing_extended_query_message = false;
 
+		/*
+		 * For valgrind reporting purposes, the "current query" begins here.
+		 */
+#ifdef USE_VALGRIND
+		old_valgrind_error_count = VALGRIND_COUNT_ERRORS;
+#endif
+
 		/*
 		 * Release storage left over from prior query cycle, and create a new
 		 * query input buffer in the cleared MessageContext.
@@ -4571,6 +4622,8 @@ PostgresMain(const char *dbname, const char *username)
 					else
 						exec_simple_query(query_string);
 
+					valgrind_report_error_query(query_string);
+
 					send_ready_for_query = true;
 				}
 				break;
@@ -4600,6 +4653,8 @@ PostgresMain(const char *dbname, const char *username)
 
 					exec_parse_message(query_string, stmt_name,
 									   paramTypes, numParams);
+
+					valgrind_report_error_query(query_string);
 				}
 				break;
 
@@ -4614,6 +4669,8 @@ PostgresMain(const char *dbname, const char *username)
 				 * the field extraction out-of-line
 				 */
 				exec_bind_message(&input_message);
+
+				/* exec_bind_message does valgrind_report_error_query */
 				break;
 
 			case 'E':			/* execute */
@@ -4631,6 +4688,8 @@ PostgresMain(const char *dbname, const char *username)
 					pq_getmsgend(&input_message);
 
 					exec_execute_message(portal_name, max_rows);
+
+					/* exec_execute_message does valgrind_report_error_query */
 				}
 				break;
 
@@ -4664,6 +4723,8 @@ PostgresMain(const char *dbname, const char *username)
 				/* commit the function-invocation transaction */
 				finish_xact_command();
 
+				valgrind_report_error_query("fastpath function call");
+
 				send_ready_for_query = true;
 				break;
 
@@ -4708,6 +4769,8 @@ PostgresMain(const char *dbname, const char *username)
 
 					if (whereToSendOutput == DestRemote)
 						pq_putemptymessage('3');	/* CloseComplete */
+
+					valgrind_report_error_query("CLOSE message");
 				}
 				break;
 
@@ -4740,6 +4803,8 @@ PostgresMain(const char *dbname, const char *username)
 											describe_type)));
 							break;
 					}
+
+					valgrind_report_error_query("DESCRIBE message");
 				}
 				break;
 
@@ -4752,6 +4817,7 @@ PostgresMain(const char *dbname, const char *username)
 			case 'S':			/* sync */
 				pq_getmsgend(&input_message);
 				finish_xact_command();
+				valgrind_report_error_query("SYNC message");
 				send_ready_for_query = true;
 				break;