RE: [EXTERNAL] Re: [PATCH] Report the query string that caused a memory error under Valgrind

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