search

pganalyze / libpg_query

C library for accessing the PostgreSQL parser outside of the server environment
BSD 3-Clause "New" or "Revised" License
1.21k stars 181 forks source link

potential memory leak? #81

Closed yingjunwu closed 3 years ago

yingjunwu commented 3 years ago

Hi I am new to libpg_query and found a potential memory leak in the library. To repro, run:

git clone -b 10-latest git://github.com/lfittl/libpg_query
cd libpg_query
make
cd example
valgrind --tool=memcheck --leak-check=yes --show-reachable=yes ./simple

output:

==14875== HEAP SUMMARY:
==14875==     in use at exit: 16,601 bytes in 3 blocks
==14875==   total heap usage: 25 allocs, 22 frees, 77,160 bytes allocated
==14875== 
==14875== 217 bytes in 1 blocks are still reachable in loss record 1 of 3
==14875==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14875==    by 0x10E7AF: MemoryContextCreate (src_backend_utils_mmgr_mcxt.c:624)
==14875==    by 0x13E717: AllocSetContextCreate (src_backend_utils_mmgr_aset.c:375)
==14875==    by 0x10E39B: MemoryContextInit (src_backend_utils_mmgr_mcxt.c:133)
==14875==    by 0x11182A: pg_query_init (pg_query.c:15)
==14875==    by 0x111848: pg_query_enter_memory_context (pg_query.c:23)
==14875==    by 0x10D9FF: pg_query_parse (pg_query_parse.c:92)
==14875==    by 0x10D740: main (simple.c:26)
==14875== 
==14875== 8,192 bytes in 1 blocks are still reachable in loss record 2 of 3
==14875==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14875==    by 0x13E367: AllocSetAlloc (src_backend_utils_mmgr_aset.c:791)
==14875==    by 0x10E5EB: MemoryContextAlloc (src_backend_utils_mmgr_mcxt.c:682)
==14875==    by 0x10E6ED: MemoryContextCreate (src_backend_utils_mmgr_mcxt.c:618)
==14875==    by 0x13E624: AllocSetContextCreate (src_backend_utils_mmgr_aset.c:375)
==14875==    by 0x10E3E3: MemoryContextInit (src_backend_utils_mmgr_mcxt.c:156)
==14875==    by 0x11182A: pg_query_init (pg_query.c:15)
==14875==    by 0x111848: pg_query_enter_memory_context (pg_query.c:23)
==14875==    by 0x10D9FF: pg_query_parse (pg_query_parse.c:92)
==14875==    by 0x10D740: main (simple.c:26)
==14875== 
==14875== 8,192 bytes in 1 blocks are still reachable in loss record 3 of 3
==14875==    at 0x4C31B0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==14875==    by 0x13E697: AllocSetContextCreate (src_backend_utils_mmgr_aset.c:418)
==14875==    by 0x10E3E3: MemoryContextInit (src_backend_utils_mmgr_mcxt.c:156)
==14875==    by 0x11182A: pg_query_init (pg_query.c:15)
==14875==    by 0x111848: pg_query_enter_memory_context (pg_query.c:23)
==14875==    by 0x10D9FF: pg_query_parse (pg_query_parse.c:92)
==14875==    by 0x10D740: main (simple.c:26)
==14875== 
==14875== LEAK SUMMARY:
==14875==    definitely lost: 0 bytes in 0 blocks
==14875==    indirectly lost: 0 bytes in 0 blocks
==14875==      possibly lost: 0 bytes in 0 blocks
==14875==    still reachable: 16,601 bytes in 3 blocks
==14875==         suppressed: 0 bytes in 0 blocks
==14875== 
==14875== For counts of detected and suppressed errors, rerun with: -v

I wonder whether this is really a memory leak? Thank you!

lfittl commented 3 years ago

@yingjunwu Thanks for reaching out & trying out libpg_query!

This is a bit complicated (I was debugging this with Valgrind the other day, in fact), and is caused by the Postgres top-level memory context, which is left allocated even after the parsing of the query (and it gets reused with the next query that gets parsed).

In the upcoming Postgres 13 branch this is addressed with an explicit function: https://github.com/lfittl/libpg_query/commit/5da2bf49512a5809fab9e45a50272e71e54c82c4

For now, I would recommend using a suppression list with Valgrind. This is not a memory leak that would grow beyond the 16kb referenced by Valgrind here, and is actually how Postgres itself works (since this memory just gets freed when Postgres shuts down).

yingjunwu commented 3 years ago

@lfittl thank you for super prompt reply! Cool I'll supress it for now and will wait for PG 13!

lfittl commented 3 years ago

@yingjunwu I've now backported this for the v10 branch as well, you can find the PR here:

https://github.com/lfittl/libpg_query/pull/83

(this also now tests that the library has a clean Valgrind run)

yingjunwu commented 3 years ago

Hi @lfittl thanks for the nice work! I may got wrong but whether it could also fix the valgrind complaint that can be repro'd using the following command:

git clone -b 10-latest git://github.com/lfittl/libpg_query
cd libpg_query
make
cd example
valgrind --tool=memcheck --leak-check=yes --show-reachable=yes ./simple

Seems that it still reports the same issue from my side (git hash: 2d0200c10cd60c1433b9433cb65c7e5df22dbd11). Yes I know it's not real mem leak :-) For now I still use valgrind.supp to suppress the false complaint.

lfittl commented 3 years ago

@yingjunwu Thats mostly because the examples currently don't call pg_query_exit(); - if you add that it should result in a clean Valgrind run.

yingjunwu commented 3 years ago

Hi @lfittl I have confirmed that valgrind no longer reports any issues after adding pg_query_exit() in simple.c. Thank you for your nice work!

lfittl commented 3 years ago

@yingjunwu Perfect, thanks for confirming - I'll close this issue.