Horton

Thread

Commits

  1. jit: Do not try to shut down LLVM state in case of LLVM triggered errors.

  1. Don't clean up LLVM state when exiting in a bad way

    Jelte Fennema-Nio <jelte.fennema@microsoft.com> — 2021-08-18T15:00:59Z

    Hi,

I ran into some segfaults when using Postgres that was compiled with LLVM 7. According to the backtraces these crashes happened during the call to llvm_shutdown, during cleanup after another out of memory condition. It seems that calls to LLVMOrcDisposeInstance, can crash (at least on LLVM 7) when LLVM is left in bad state. I attached the relevant part of the stacktrace to this email.

With the attached patch these segfaults went away. The patch turns llvm_shutdown into a no-op whenever the backend is exiting with an error. Based on my understanding of the code this should be totally fine. No memory should be leaked, since all memory will be cleaned up anyway once the backend exits shortly after. The only reason this cleanup code even seems to exist at all is to get useful LLVM profiling data. To me it seems be acceptable if the profiling data is incorrect/missing when the backend exits with an error.

Jelte

  2. Re: Don't clean up LLVM state when exiting in a bad way

    Zhihong Yu <zyu@yugabyte.com> — 2021-08-18T15:17:54Z

    On Wed, Aug 18, 2021 at 8:01 AM Jelte Fennema <Jelte.Fennema@microsoft.com>
wrote:

> Hi,
>
> I ran into some segfaults when using Postgres that was compiled with LLVM
> 7. According to the backtraces these crashes happened during the call to
> llvm_shutdown, during cleanup after another out of memory condition. It
> seems that calls to LLVMOrcDisposeInstance, can crash (at least on LLVM 7)
> when LLVM is left in bad state. I attached the relevant part of the
> stacktrace to this email.
>
> With the attached patch these segfaults went away. The patch turns
> llvm_shutdown into a no-op whenever the backend is exiting with an error.
> Based on my understanding of the code this should be totally fine. No
> memory should be leaked, since all memory will be cleaned up anyway once
> the backend exits shortly after. The only reason this cleanup code even
> seems to exist at all is to get useful LLVM profiling data. To me it seems
> be acceptable if the profiling data is incorrect/missing when the backend
> exits with an error.
>
> Jelte
>
Hi,
Minor comment:

+        * shut LLVM down, this can result into a segfault. So if this
process

result into a segfault -> result in a segfault

Cheers

  3. Re: Don't clean up LLVM state when exiting in a bad way

    Justin Pryzby <pryzby@telsasoft.com> — 2021-09-06T02:33:56Z

    On Wed, Aug 18, 2021 at 03:00:59PM +0000, Jelte Fennema wrote:
> I ran into some segfaults when using Postgres that was compiled with LLVM 7. According to the backtraces these crashes happened during the call to llvm_shutdown, during cleanup after another out of memory condition. It seems that calls to LLVMOrcDisposeInstance, can crash (at least on LLVM 7) when LLVM is left in bad state. I attached the relevant part of the stacktrace to this email.
> 
> With the attached patch these segfaults went away. The patch turns llvm_shutdown into a no-op whenever the backend is exiting with an error. Based on my understanding of the code this should be totally fine. No memory should be leaked, since all memory will be cleaned up anyway once the backend exits shortly after. The only reason this cleanup code even seems to exist at all is to get useful LLVM profiling data. To me it seems be acceptable if the profiling data is incorrect/missing when the backend exits with an error.

Andres , could you comment on this ?

This seems to explain the crash I reported to you when testing your WIP patches
for the JIT memory leak.  I realize now that the crash happens without your
patches.
https://www.postgresql.org/message-id/20210419164130.byegpfrw46mzagcu@alap3.anarazel.de

I can reproduce the crash on master (not just v13, as I said before) compiled
on centos7, with:
LLVM_CONFIG=/usr/lib64/llvm7.0/bin/llvm-config CLANG=/opt/rh/llvm-toolset-7.0/root/usr/bin/clang

I cannot reproduce the crash after applying Jelte's patch.

I couldn't crash on ubuntu either, so maybe they have a patch which fixes this,
or maybe RH applied a patch which caused it...

postgres=# CREATE TABLE t AS SELECT i FROM generate_series(1,999999)i; VACUUM ANALYZE t;
postgres=# SET client_min_messages=debug; SET statement_timeout=333; SET jit_above_cost=0; SET jit_optimize_above_cost=-1; SET jit_inline_above_cost=-1; explain analyze SELECT sum(i) FROM t a NATURAL JOIN t b;
2021-09-05 22:47:12.807 ADT client backend[7563] psql ERROR:  canceling statement due to statement timeout
2021-09-05 22:47:12.880 ADT postmaster[7272] LOG:  background worker "parallel worker" (PID 8212) was terminated by signal 11: Segmentation fault

-- 
Justin

  4. Re: Don't clean up LLVM state when exiting in a bad way

    Andres Freund <andres@anarazel.de> — 2021-09-07T19:27:27Z

    Hi,

On 2021-08-18 15:00:59 +0000, Jelte Fennema wrote:
> I ran into some segfaults when using Postgres that was compiled with LLVM
> 7. According to the backtraces these crashes happened during the call to
> llvm_shutdown, during cleanup after another out of memory condition. It
> seems that calls to LLVMOrcDisposeInstance, can crash (at least on LLVM 7)
> when LLVM is left in bad state. I attached the relevant part of the
> stacktrace to this email.

> With the attached patch these segfaults went away. The patch turns
> llvm_shutdown into a no-op whenever the backend is exiting with an
> error. Based on my understanding of the code this should be totally fine. No
> memory should be leaked, since all memory will be cleaned up anyway once the
> backend exits shortly after. The only reason this cleanup code even seems to
> exist at all is to get useful LLVM profiling data. To me it seems be
> acceptable if the profiling data is incorrect/missing when the backend exits
> with an error.

I think this is a tad too strong. We should continue to clean up on exit as
long as the error didn't happen while we're already inside llvm
code. Otherwise we loose some ability to find leaks. How about checking in the
error path whether fatal_new_handler_depth is > 0, and skipping cleanup in
that case? Because that's precisely when it should be unsafe to reenter LLVM.

Greetings,

Andres Freund

  5. Re: Don't clean up LLVM state when exiting in a bad way

    Justin Pryzby <pryzby@telsasoft.com> — 2021-09-07T19:44:39Z

    On Tue, Sep 07, 2021 at 12:27:27PM -0700, Andres Freund wrote:
> Hi,
> 
> On 2021-08-18 15:00:59 +0000, Jelte Fennema wrote:
> > I ran into some segfaults when using Postgres that was compiled with LLVM
> > 7. According to the backtraces these crashes happened during the call to
> > llvm_shutdown, during cleanup after another out of memory condition. It
> > seems that calls to LLVMOrcDisposeInstance, can crash (at least on LLVM 7)
> > when LLVM is left in bad state. I attached the relevant part of the
> > stacktrace to this email.
> 
> > With the attached patch these segfaults went away. The patch turns
> > llvm_shutdown into a no-op whenever the backend is exiting with an
> > error. Based on my understanding of the code this should be totally fine. No
> > memory should be leaked, since all memory will be cleaned up anyway once the
> > backend exits shortly after. The only reason this cleanup code even seems to
> > exist at all is to get useful LLVM profiling data. To me it seems be
> > acceptable if the profiling data is incorrect/missing when the backend exits
> > with an error.
> 
> I think this is a tad too strong. We should continue to clean up on exit as
> long as the error didn't happen while we're already inside llvm
> code. Otherwise we loose some ability to find leaks. How about checking in the
> error path whether fatal_new_handler_depth is > 0, and skipping cleanup in
> that case? Because that's precisely when it should be unsafe to reenter LLVM.

This avoids a crash when compiled with llvm7+clang7 on RH7 on master:

python3 -c "import pg; db=pg.DB('dbname=postgres host=/tmp port=5678'); db.query('SET jit_above_cost=0; SET jit_inline_above_cost=-1; SET jit=on; SET client_min_messages=debug'); db.query('begin'); db.query_formatted('SELECT 1 FROM generate_series(1,99)a WHERE a=%s', [1], inline=False);"

diff --git a/src/backend/jit/llvm/llvmjit.c b/src/backend/jit/llvm/llvmjit.c
index df691cbf1c..a3869ee700 100644
--- a/src/backend/jit/llvm/llvmjit.c
+++ b/src/backend/jit/llvm/llvmjit.c
@@ -885,6 +885,12 @@ llvm_session_initialize(void)
 static void
 llvm_shutdown(int code, Datum arg)
 {
+	extern int fatal_new_handler_depth;
+
+	// if (code!=0)
+	if (fatal_new_handler_depth > 0)
+		return;
+
 #if LLVM_VERSION_MAJOR > 11
 	{
 		if (llvm_opt3_orc)
diff --git a/src/backend/jit/llvm/llvmjit_error.cpp b/src/backend/jit/llvm/llvmjit_error.cpp
index 26bc828875..802dc1b058 100644
--- a/src/backend/jit/llvm/llvmjit_error.cpp
+++ b/src/backend/jit/llvm/llvmjit_error.cpp
@@ -24,7 +24,7 @@ extern "C"
 #include "jit/llvmjit.h"
 
 
-static int fatal_new_handler_depth = 0;
+int fatal_new_handler_depth = 0;
 static std::new_handler old_new_handler = NULL;
 
 static void fatal_system_new_handler(void);

  6. Re: Don't clean up LLVM state when exiting in a bad way

    Andres Freund <andres@anarazel.de> — 2021-09-14T01:32:55Z

    Hi,


On 2021-09-07 14:44:39 -0500, Justin Pryzby wrote:
> On Tue, Sep 07, 2021 at 12:27:27PM -0700, Andres Freund wrote:
> > I think this is a tad too strong. We should continue to clean up on exit as
> > long as the error didn't happen while we're already inside llvm
> > code. Otherwise we loose some ability to find leaks. How about checking in the
> > error path whether fatal_new_handler_depth is > 0, and skipping cleanup in
> > that case? Because that's precisely when it should be unsafe to reenter
> > LLVM.

The more important reason is actually profiling information that needs to be
written out.

I've now pushed a fix to all relevant branches. Thanks all!

Regards,

Andres

  7. Re: Don't clean up LLVM state when exiting in a bad way

    Alexander Law <exclusion@gmail.com> — 2021-09-14T04:00:00Z

    Hello hackers,
14.09.2021 04:32, Andres Freund wrote:
> On 2021-09-07 14:44:39 -0500, Justin Pryzby wrote:
>> On Tue, Sep 07, 2021 at 12:27:27PM -0700, Andres Freund wrote:
>>> I think this is a tad too strong. We should continue to clean up on exit as
>>> long as the error didn't happen while we're already inside llvm
>>> code. Otherwise we loose some ability to find leaks. How about checking in the
>>> error path whether fatal_new_handler_depth is > 0, and skipping cleanup in
>>> that case? Because that's precisely when it should be unsafe to reenter
>>> LLVM.
> The more important reason is actually profiling information that needs to be
> written out.
>
> I've now pushed a fix to all relevant branches. Thanks all!
>
I've encountered similar issue last week, but found this discussion only
after the commit.
I'm afraid that it's not completely gone yet. I've reproduced a similar
crash (on edb4d95d) with
echo "statement_timeout = 50
jit_optimize_above_cost = 1
jit_inline_above_cost = 1
parallel_setup_cost=0
parallel_tuple_cost=0
" >/tmp/extra.config
TEMP_CONFIG=/tmp/extra.config  make check

parallel group (11 tests):  memoize explain hash_part partition_info
reloptions tuplesort compression partition_aggregate indexing
partition_prune partition_join
     partition_join               ... FAILED (test process exited with
exit code 2)     1815 ms
     partition_prune              ... FAILED (test process exited with
exit code 2)     1779 ms
     reloptions                   ... ok          146 ms

I've extracted the crash-causing fragment from the partition_prune test
to reproduce the segfault reliably (see the patch attached).
The segfault stack is:
Core was generated by `postgres: parallel worker for PID
12029                                       '.
Program terminated with signal 11, Segmentation fault.
#0  0x00007f045e0a88ca in notifyFreed (K=<optimized out>, Obj=...,
this=<optimized out>)
    at
/usr/src/debug/llvm-7.0.1.src/lib/ExecutionEngine/Orc/OrcCBindingsStack.h:485
485           Listener->NotifyFreeingObject(Obj);
(gdb) bt
#0  0x00007f045e0a88ca in notifyFreed (K=<optimized out>, Obj=...,
this=<optimized out>)
    at
/usr/src/debug/llvm-7.0.1.src/lib/ExecutionEngine/Orc/OrcCBindingsStack.h:485
#1  operator() (K=<optimized out>, Obj=..., __closure=<optimized out>)
    at
/usr/src/debug/llvm-7.0.1.src/lib/ExecutionEngine/Orc/OrcCBindingsStack.h:226
#2  std::_Function_handler<void (unsigned long, llvm::object::ObjectFile
const&),
llvm::OrcCBindingsStack::OrcCBindingsStack(llvm::TargetMachine&,
std::function<std::unique_ptr<llvm::orc::IndirectStubsManager,
std::default_delete<llvm::orc::IndirectStubsManager> >
()>)::{lambda(unsigned long, llvm::object::ObjectFile
const&)#3}>::_M_invoke(std::_Any_data const&, unsigned long,
llvm::object::ObjectFile const&) (__functor=..., __args#0=<optimized
out>, __args#1=...)
    at /usr/include/c++/4.8.2/functional:2071
#3  0x00007f045e0aa578 in operator() (__args#1=..., __args#0=<optimized
out>, this=<optimized out>)
    at /usr/include/c++/4.8.2/functional:2471
...

The corresponding code in OrcCBindingsStack.h is:
void notifyFreed(orc::VModuleKey K, const object::ObjectFile &Obj) {
    for (auto &Listener : EventListeners)
     Listener->NotifyFreeingObject(Obj);
}
So probably one of the EventListeners has become null. I see that
without debugging and profiling enabled the only listener registration
in the postgres code is LLVMOrcRegisterJITEventListener.

With LLVM 9 on the same Centos 7 I don't get such segfault. Also it
doesn't happen on different OSes with LLVM 7. I still have no
explanation for that, but maybe there is difference between LLVM
configure options, e.g. like this:
https://stackoverflow.com/questions/47712670/segmentation-fault-in-llvm-pass-when-using-registerstandardpasses

Best regards,
Alexander

  8. Re: Don't clean up LLVM state when exiting in a bad way

    Andres Freund <andres@anarazel.de> — 2021-09-14T05:05:23Z

    Hi, 

On September 13, 2021 9:00:00 PM PDT, Alexander Lakhin <exclusion@gmail.com> wrote:
>Hello hackers,
>14.09.2021 04:32, Andres Freund wrote:
>> On 2021-09-07 14:44:39 -0500, Justin Pryzby wrote:
>>> On Tue, Sep 07, 2021 at 12:27:27PM -0700, Andres Freund wrote:
>>>> I think this is a tad too strong. We should continue to clean up on exit as
>>>> long as the error didn't happen while we're already inside llvm
>>>> code. Otherwise we loose some ability to find leaks. How about checking in the
>>>> error path whether fatal_new_handler_depth is > 0, and skipping cleanup in
>>>> that case? Because that's precisely when it should be unsafe to reenter
>>>> LLVM.
>> The more important reason is actually profiling information that needs to be
>> written out.
>>
>> I've now pushed a fix to all relevant branches. Thanks all!
>>
>I've encountered similar issue last week, but found this discussion only
>after the commit.
>I'm afraid that it's not completely gone yet. I've reproduced a similar
>crash (on edb4d95d) with
>echo "statement_timeout = 50
>jit_optimize_above_cost = 1
>jit_inline_above_cost = 1
>parallel_setup_cost=0
>parallel_tuple_cost=0
>" >/tmp/extra.config
>TEMP_CONFIG=/tmp/extra.config  make check
>
>parallel group (11 tests):  memoize explain hash_part partition_info
>reloptions tuplesort compression partition_aggregate indexing
>partition_prune partition_join
>     partition_join               ... FAILED (test process exited with
>exit code 2)     1815 ms
>     partition_prune              ... FAILED (test process exited with
>exit code 2)     1779 ms
>     reloptions                   ... ok          146 ms
>
>I've extracted the crash-causing fragment from the partition_prune test
>to reproduce the segfault reliably (see the patch attached).
>The segfault stack is:
>Core was generated by `postgres: parallel worker for PID
>12029                                       '.
>Program terminated with signal 11, Segmentation fault.
>#0  0x00007f045e0a88ca in notifyFreed (K=<optimized out>, Obj=...,
>this=<optimized out>)
>    at
>/usr/src/debug/llvm-7.0.1.src/lib/ExecutionEngine/Orc/OrcCBindingsStack.h:485
>485           Listener->NotifyFreeingObject(Obj);
>(gdb) bt
>#0  0x00007f045e0a88ca in notifyFreed (K=<optimized out>, Obj=...,
>this=<optimized out>)
>    at
>/usr/src/debug/llvm-7.0.1.src/lib/ExecutionEngine/Orc/OrcCBindingsStack.h:485
>#1  operator() (K=<optimized out>, Obj=..., __closure=<optimized out>)
>    at
>/usr/src/debug/llvm-7.0.1.src/lib/ExecutionEngine/Orc/OrcCBindingsStack.h:226
>#2  std::_Function_handler<void (unsigned long, llvm::object::ObjectFile
>const&),
>llvm::OrcCBindingsStack::OrcCBindingsStack(llvm::TargetMachine&,
>std::function<std::unique_ptr<llvm::orc::IndirectStubsManager,
>std::default_delete<llvm::orc::IndirectStubsManager> >
>()>)::{lambda(unsigned long, llvm::object::ObjectFile
>const&)#3}>::_M_invoke(std::_Any_data const&, unsigned long,
>llvm::object::ObjectFile const&) (__functor=..., __args#0=<optimized
>out>, __args#1=...)
>    at /usr/include/c++/4.8.2/functional:2071
>#3  0x00007f045e0aa578 in operator() (__args#1=..., __args#0=<optimized
>out>, this=<optimized out>)
>    at /usr/include/c++/4.8.2/functional:2471
>...
>
>The corresponding code in OrcCBindingsStack.h is:
>void notifyFreed(orc::VModuleKey K, const object::ObjectFile &Obj) {
>    for (auto &Listener : EventListeners)
>     Listener->NotifyFreeingObject(Obj);
>}
>So probably one of the EventListeners has become null. I see that
>without debugging and profiling enabled the only listener registration
>in the postgres code is LLVMOrcRegisterJITEventListener.
>
>With LLVM 9 on the same Centos 7 I don't get such segfault. Also it
>doesn't happen on different OSes with LLVM 7.

That just like an llvm bug to me. Rather than the usage issue addressed in this thread.


 I still have no
>explanation for that, but maybe there is difference between LLVM
>configure options, e.g. like this:
>https://stackoverflow.com/questions/47712670/segmentation-fault-in-llvm-pass-when-using-registerstandardpasses

Why is it not much more likely that bugs were fixed?


Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  9. Re: Don't clean up LLVM state when exiting in a bad way

    Alexander Law <exclusion@gmail.com> — 2021-09-14T20:00:00Z

    Hello Andres,
14.09.2021 08:05, Andres Freund wrote:
>
>> With LLVM 9 on the same Centos 7 I don't get such segfault. Also it
>> doesn't happen on different OSes with LLVM 7.
> That just like an llvm bug to me. Rather than the usage issue addressed in this thread.
But Justin seen this strangeness too:
>
> I couldn't crash on ubuntu either, so maybe they have a patch which
> fixes this,
> or maybe RH applied a patch which caused it...
>
The script that Justin presented:
> postgres=# CREATE TABLE t AS SELECT i FROM generate_series(1,999999)i;
> VACUUM ANALYZE t;
> postgres=# SET client_min_messages=debug; SET statement_timeout=333;
> SET jit_above_cost=0; SET jit_optimize_above_cost=-1; SET
> jit_inline_above_cost=-1; explain analyze SELECT sum(i) FROM t a
> NATURAL JOIN t b;
causes the server crash on Centos 7 with LLVM 7 (even with the fix
applied) and doesn't crash with LLVM 9 (I used llvm-toolset-9* and
devtoolset-9* packages from
https://buildlogs.centos.org/c7-llvm-toolset-9.0.x86_64/ and
https://buildlogs.centos.org/c7-devtoolset-9.x86_64/ repositories).

The another script:
>
> python3 -c "import pg; db=pg.DB('dbname=postgres host=/tmp
> port=5678'); db.query('SET jit_above_cost=0; SET
> jit_inline_above_cost=-1; SET jit=on; SET client_min_messages=debug');
> db.query('begin'); db.query_formatted('SELECT 1 FROM
> generate_series(1,99)a WHERE a=%s', [1], inline=False);"
>
also causes the server crash with LLVM 7, and doesn't crash with LLVM 9.

So I wonder, isn't the fixed usage issue specific to LLVM 7, which is
not going to be supported as having some bugs?

Best regards,
Alexander

  10. Re: [EXTERNAL] Re: Don't clean up LLVM state when exiting in a bad way

    Jelte Fennema-Nio <jelte.fennema@microsoft.com> — 2021-09-14T20:36:44Z

    > So I wonder, isn't the fixed usage issue specific to LLVM 7

That's definitely possible. I was unable to reproduce the issue I shared in my original email when postgres was compiled with LLVM 10. 

That's also why I sent an email to the pgsql-pkg-yum mailing list about options to use a newer version of LLVM on CentOS 7: https://www.postgresql.org/message-id/flat/AM5PR83MB0178475D87EFA290A4D0793DF7FF9%40AM5PR83MB0178.EURPRD83.prod.outlook.com (no response so far though)