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 Sent: Sunday, April 2, 2023 11:14 PM To: Onur Tirtir Cc: peter.eisentr...@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 t...@sss.pgh.pa.us. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Onur Tirtir 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
RE: [EXTERNAL] Re: [PATCH] Report the query string that caused a memory error under Valgrind
Hey Peter, Thank you for reviewing the patch and for your feedback. I believe the v2 patch should be able to handle other protocol messages too. -Original Message- From: Peter Eisentraut Sent: Wednesday, March 22, 2023 7:00 PM To: Onur Tirtir ; 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.eisentr...@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 protocol should be handled as well. Maybe it would be better to move this up to PostgresMain() and handle all protocol messages? v2-0001-Report-the-query-string-that-caused-a-memory-erro.patch Description: v2-0001-Report-the-query-string-that-caused-a-memory-erro.patch
[PATCH] Report the query string that caused a memory error under Valgrind
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. An example piece of valgrind test output for a memory error found in Citus is as follows: ==67222== VALGRINDERROR-BEGIN ==67222== Invalid write of size 8 ==67222==at 0x7A6F040: dlist_delete (home/pguser/postgres-installation/include/postgresql/server/lib/ilist.h:360) ==67222==by 0x7A6F040: ResetRemoteTransaction (home/pguser/citus/src/backend/distributed/transaction/remote_transaction.c:872) ==67222==by 0x79CF606: AfterXactHostConnectionHandling (home/pguser/citus/src/backend/distributed/connection/connection_management.c:1468) ==67222==by 0x79CF65E: AfterXactConnectionHandling (home/pguser/citus/src/backend/distributed/connection/connection_management.c:175) ==67222==by 0x7A6FEDA: CoordinatedTransactionCallback (home/pguser/citus/src/backend/distributed/transaction/transaction_management.c:309) ==67222==by 0x544F30: CallXactCallbacks (home/pguser/postgres-source/postgresql-15.1/src/backend/access/transam/xact.c:3661) ==67222==by 0x548E12: CommitTransaction (home/pguser/postgres-source/postgresql-15.1/src/backend/access/transam/xact.c:2298) ==67222==by 0x549BBC: CommitTransactionCommand (home/pguser/postgres-source/postgresql-15.1/src/backend/access/transam/xact.c:3048) ==67222==by 0x832C30: finish_xact_command (home/pguser/postgres-source/postgresql-15.1/src/backend/tcop/postgres.c:2750) ==67222==by 0x8352AF: exec_simple_query (home/pguser/postgres-source/postgresql-15.1/src/backend/tcop/postgres.c:1279) ==67222==by 0x837312: PostgresMain (home/pguser/postgres-source/postgresql-15.1/src/backend/tcop/postgres.c:4595) ==67222==by 0x79F7B5: BackendRun (home/pguser/postgres-source/postgresql-15.1/src/backend/postmaster/postmaster.c:4504) ==67222==by 0x7A24E6: BackendStartup (home/pguser/postgres-source/postgresql-15.1/src/backend/postmaster/postmaster.c:4232) ==67222== Address 0x7486378 is 3,512 bytes inside a recently re-allocated block of size 8,192 alloc'd ==67222==at 0x484486F: malloc (builddir/build/BUILD/valgrind-3.19.0/coregrind/m_replacemalloc/vg_replace_malloc.c:381) ==67222==by 0x98B6EB: AllocSetContextCreateInternal (home/pguser/postgres-source/postgresql-15.1/src/backend/utils/mmgr/aset.c:469) ==67222==by 0x79CEABA: InitializeConnectionManagement (home/pguser/citus/src/backend/distributed/connection/connection_management.c:107) ==67222==by 0x799FE9F: _PG_init (home/pguser/citus/src/backend/distributed/shared_library_init.c:464) ==67222==by 0x96AE6B: internal_load_library (home/pguser/postgres-source/postgresql-15.1/src/backend/utils/fmgr/dfmgr.c:289) ==67222==by 0x96B09A: load_file (home/pguser/postgres-source/postgresql-15.1/src/backend/utils/fmgr/dfmgr.c:156) ==67222==by 0x973122: load_libraries (home/pguser/postgres-source/postgresql-15.1/src/backend/utils/init/miscinit.c:1668) ==67222==by 0x974680: process_shared_preload_libraries (home/pguser/postgres-source/postgresql-15.1/src/backend/utils/init/miscinit.c:1686) ==67222==by 0x7A336A: PostmasterMain (home/pguser/postgres-source/postgresql-15.1/src/backend/postmaster/postmaster.c:1026) ==67222==by 0x6F303C: main (home/pguser/postgres-source/postgresql-15.1/src/backend/main/main.c:202) ==67222== ==67222== VALGRINDERROR-END **67222** The query for which valgrind reported a memory error was: REFRESH MATERIALIZED VIEW other_schema.mat_view; v1-0001-Report-the-query-string-that-caused-a-mem-error.patch Description: v1-0001-Report-the-query-string-that-caused-a-mem-error.patch
