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

On 31.01.23 15:00, Onur Tirtir wrote:
> We use Valgrind --together with the suppression file provided in 
> Postgres repo-- to test Citus extension against memory errors.
> 
> We replace /bin/postgres executable with a simple bash script that 
> executes the original postgres executable under Valgrind and then we run 
> our usual regression tests.
> 
> However, it is quite hard to understand which query caused a memory 
> error in the stack traces that has been dumped into valgrind logfile.
> 
> For this reason, we propose the attached patch to allow Valgrind to 
> report the query string that caused a memory error right after the 
> relevant stack trace.
> 
> I belive this would not only be useful for Citus but also for Postgres 
> and other extensions in their valgrind-testing process.

I can see how this could be useful.  But this only handles queries using 
the simple protocol.  At least the extended protocol should be handled 
as well.  Maybe it would be better to move this up to PostgresMain() and 
handle all protocol messages?

Hey Peter,

Thank you for reviewing the patch and for your feedback. I believe the v2 patch should be able to handle other protocol
messagestoo. 

-----Original Message-----
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Sent: Wednesday, March 22, 2023 7:00 PM
To: Onur Tirtir <Onur.Tirtir@microsoft.com>; pgsql-hackers@lists.postgresql.org
Subject: [EXTERNAL] Re: [PATCH] Report the query string that caused a memory error under Valgrind

[You don't often get email from peter.eisentraut@enterprisedb.com. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification] 

On 31.01.23 15:00, Onur Tirtir wrote:
> We use Valgrind --together with the suppression file provided in
> Postgres repo-- to test Citus extension against memory errors.
>
> We replace /bin/postgres executable with a simple bash script that
> executes the original postgres executable under Valgrind and then we
> run our usual regression tests.
>
> However, it is quite hard to understand which query caused a memory
> error in the stack traces that has been dumped into valgrind logfile.
>
> For this reason, we propose the attached patch to allow Valgrind to
> report the query string that caused a memory error right after the
> relevant stack trace.
>
> I belive this would not only be useful for Citus but also for Postgres
> and other extensions in their valgrind-testing process.

I can see how this could be useful.  But this only handles queries using the simple protocol.  At least the extended
protocolshould be handled as well.  Maybe it would be better to move this up to PostgresMain() and handle all protocol
messages?

Onur Tirtir <Onur.Tirtir@microsoft.com> writes:
> Thank you for reviewing the patch and for your feedback. I believe the v2 patch should be able to handle other
protocolmessages too. 

I like the concept here, but the reporting that the v2 patch provides
would be seriously horrid: it's trying to print stuff that isn't
necessarily text, and for bind and execute messages it's substantially
dumber than the existing debug_query_string infrastructure.  Another
thing that is not great is that if Postgres itself throws an error
later in the query, nothing will be reported since we don't reach the
bottom of the processing loop.

I suggest that we need something closer to the attached.  Some
bikeshedding is possible on the specific printouts, but I'm not
sure it's worth working harder than this.

            regards, tom lane

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;

Hey Tom,

Thank you for sharing your proposal as a patch. It looks much nicer and useful than mine.
I've also tested it for a few cases --by injecting a memory error on purpose-- and seen that it helps reporting the
problematicquery. 
Should we move forward with v3 then?

==13210== VALGRINDERROR-BEGIN
==13210== Conditional jump or move depends on uninitialised value(s)
==13210==    at 0x75B88C: exec_simple_query (home/onurctirtir/postgres/src/backend/tcop/postgres.c:1070)
==13210==    by 0x760ABB: PostgresMain (home/onurctirtir/postgres/src/backend/tcop/postgres.c:4624)
==13210==    by 0x688F1A: BackendRun (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:4461)
==13210==    by 0x688801: BackendStartup (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:4189)
==13210==    by 0x684D21: ServerLoop (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:1779)
==13210==    by 0x6845F6: PostmasterMain (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:1463)
==13210==    by 0x540351: main (home/onurctirtir/postgres/src/backend/main/main.c:200)
==13210==  Uninitialised value was created by a heap allocation
==13210==    at 0x483B7F3: malloc (in /usr/lib/x86_64-linux-gnu/valgrind/vgpreload_memcheck-amd64-linux.so)
==13210==    by 0x75B812: exec_simple_query (home/onurctirtir/postgres/src/backend/tcop/postgres.c:1023)
==13210==    by 0x760ABB: PostgresMain (home/onurctirtir/postgres/src/backend/tcop/postgres.c:4624)
==13210==    by 0x688F1A: BackendRun (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:4461)
==13210==    by 0x688801: BackendStartup (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:4189)
==13210==    by 0x684D21: ServerLoop (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:1779)
==13210==    by 0x6845F6: PostmasterMain (home/onurctirtir/postgres/src/backend/postmaster/postmaster.c:1463)
==13210==    by 0x540351: main (home/onurctirtir/postgres/src/backend/main/main.c:200)
==13210==
==13210== VALGRINDERROR-END
**13210** Valgrind detected 1 error(s) during execution of "select 1;"
**13210** Valgrind detected 1 error(s) during execution of "select 1;"

Best, Onur

-----Original Message-----
From: Tom Lane <tgl@sss.pgh.pa.us>
Sent: Sunday, April 2, 2023 11:14 PM
To: Onur Tirtir <Onur.Tirtir@microsoft.com>
Cc: peter.eisentraut@enterprisedb.com; pgsql-hackers@lists.postgresql.org
Subject: Re: [EXTERNAL] Re: [PATCH] Report the query string that caused a memory error under Valgrind

[You don't often get email from tgl@sss.pgh.pa.us. Learn why this is important at
https://aka.ms/LearnAboutSenderIdentification] 

Onur Tirtir <Onur.Tirtir@microsoft.com> writes:
> Thank you for reviewing the patch and for your feedback. I believe the v2 patch should be able to handle other
protocolmessages too. 

I like the concept here, but the reporting that the v2 patch provides would be seriously horrid: it's trying to print
stuffthat isn't necessarily text, and for bind and execute messages it's substantially dumber than the existing
debug_query_stringinfrastructure.  Another thing that is not great is that if Postgres itself throws an error later in
thequery, nothing will be reported since we don't reach the bottom of the processing loop. 

I suggest that we need something closer to the attached.  Some bikeshedding is possible on the specific printouts, but
I'mnot sure it's worth working harder than this. 

                        regards, tom lane

Onur Tirtir <Onur.Tirtir@microsoft.com> writes:
> Thank you for sharing your proposal as a patch. It looks much nicer and useful than mine.
> I've also tested it for a few cases --by injecting a memory error on purpose-- and seen that it helps reporting the
problematicquery. 
> Should we move forward with v3 then?

OK, I pushed v3 as-is.  We can refine it later if anyone has suggestions.

Thanks for the contribution!

            regards, tom lane