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 problematic query.
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(at)sss(dot)pgh(dot)pa(dot)us>
Sent: Sunday, April 2, 2023 11:14 PM
To: Onur Tirtir <Onur(dot)Tirtir(at)microsoft(dot)com>
Cc: peter(dot)eisentraut(at)enterprisedb(dot)com; pgsql-hackers(at)lists(dot)postgresql(dot)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(at)sss(dot)pgh(dot)pa(dot)us(dot) Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]

Onur Tirtir <Onur(dot)Tirtir(at)microsoft(dot)com> writes:
> Thank you for reviewing the patch and for your feedback. I believe the v2 patch should be able to handle other protocol messages 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