Horton

Thread

Commits

  1. LLVMJIT: Release JIT context after running ExprContext shutdown callbacks.

  1. LLVM jit and matview

    Dmitry Dolgov <9erthalion6@gmail.com> — 2018-07-09T14:04:11Z

    Hi,

I've found out an interesting problem that you can reproduce by running
installcheck against a database with enabled llvm jit (with and without the
patch I've sent in [1]):

# postgresql.conf
jit = on
jit_above_cost = 0
jit_optimize_above_cost = 0
jit_inline_above_cost = 0

# matview.sql
...
=# REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_tm;
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.

# gdb
Program received signal SIGSEGV, Segmentation fault.
0x00007f5fa2c24f80 in ?? ()
Traceback (most recent call last):
File "<string>", line 616, in lines
File "<string>", line 113, in run
gdb.error: No function contains program counter for selected frame.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "<string>", line 180, in <lambda>
File "<string>", line 191, in on_stop
File "<string>", line 222, in display
File "<string>", line 633, in lines
gdb.MemoryError: Cannot access memory at address 0x7f5fa2c24f80
>>> bt
#0  0x00007f5fa2c24f80 in ?? ()
#1  0x0000000000648218 in ShutdownExprContext
(econtext=econtext@entry=0x2988a20, isCommit=isCommit@entry=true) at
execUtils.c:851
#2  0x000000000064877e in FreeExprContext (econtext=0x2988a20,
isCommit=isCommit@entry=true) at execUtils.c:364
#3  0x00000000006487e0 in FreeExecutorState
(estate=estate@entry=0x2986bd8) at execUtils.c:202
#4  0x000000000063ed4e in standard_ExecutorEnd (queryDesc=0x3139100)
at execMain.c:514
#5  0x000000000063ed96 in ExecutorEnd
(queryDesc=queryDesc@entry=0x3139100) at execMain.c:466
#6  0x0000000000670c11 in _SPI_pquery
(queryDesc=queryDesc@entry=0x3139100,
fire_triggers=fire_triggers@entry=true, tcount=1) at spi.c:2455
#7  0x0000000000672d42 in _SPI_execute_plan
(plan=plan@entry=0x7ffef6c89b40, paramLI=paramLI@entry=0x0,
snapshot=snapshot@entry=0x0,
crosscheck_snapshot=crosscheck_snapshot@entry=0x0,
read_only=read_only@entry=false,
fire_triggers=fire_triggers@entry=true, tcount=1) at spi.c:2204
#8  0x00000000006730ba in SPI_execute (src=0x2c947f8 "SELECT newdata
FROM pg_temp_3.pg_temp_16397 newdata WHERE newdata IS NOT NULL AND
EXISTS (SELECT 1 FROM pg_temp_3.pg_temp_16397 newdata2 WHERE newdata2
IS NOT NULL AND newdata2 OPERATOR(pg_catalog.*=)"...,
read_only=read_only@entry=false, tcount=tcount@entry=1) at spi.c:418
#9  0x00000000005e52f2 in refresh_by_match_merge
(matviewOid=matviewOid@entry=16397, tempOid=tempOid@entry=16460,
relowner=relowner@entry=10, save_sec_context=0) at matview.c:632
#10 0x00000000005e619e in ExecRefreshMatView
(stmt=stmt@entry=0x1b83100, queryString=queryString@entry=0x1b82668
"REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_tm;",
params=params@entry=0x0,
completionTag=completionTag@entry=0x7ffef6c8a3c0 "") at matview.c:323
#11 0x00000000007a7926 in ProcessUtilitySlow
(pstate=pstate@entry=0x28a1d08, pstmt=pstmt@entry=0x1b831c0,
queryString=queryString@entry=0x1b82668 "REFRESH MATERIALIZED VIEW
CONCURRENTLY mvtest_tm;",
context=context@entry=PROCESS_UTILITY_TOPLEVEL,
params=params@entry=0x0, queryEnv=queryEnv@entry=0x0, dest=0x1b834b0,
completionTag=0x7ffef6c8a3c0 "") at utility.c:1493
#12 0x00000000007a69d5 in standard_ProcessUtility (pstmt=0x1b831c0,
queryString=0x1b82668 "REFRESH MATERIALIZED VIEW CONCURRENTLY
mvtest_tm;", context=PROCESS_UTILITY_TOPLEVEL, params=0x0,
queryEnv=0x0, dest=0x1b834b0, completionTag=0x7ffef6c8a3c0 "") at
utility.c:920
#13 0x00000000007a6a8d in ProcessUtility (pstmt=pstmt@entry=0x1b831c0,
queryString=<optimized out>, context=<optimized out>,
params=<optimized out>, queryEnv=<optimized out>,
dest=dest@entry=0x1b834b0, completionTag=0x7ffef6c8a3c0 "") at
utility.c:360
#14 0x00000000007a3162 in PortalRunUtility
(portal=portal@entry=0x1be6af8, pstmt=pstmt@entry=0x1b831c0,
isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x1b834b0,
completionTag=completionTag@entry=0x7ffef6c8a3c0 "") at pquery.c:1178
#15 0x00000000007a3cd6 in PortalRunMulti
(portal=portal@entry=0x1be6af8, isTopLevel=isTopLevel@entry=true,
setHoldSnapshot=setHoldSnapshot@entry=false,
dest=dest@entry=0x1b834b0, altdest=altdest@entry=0x1b834b0,
completionTag=completionTag@entry=0x7ffef6c8a3c0 "") at pquery.c:1324
#16 0x00000000007a4ac8 in PortalRun (portal=portal@entry=0x1be6af8,
count=count@entry=9223372036854775807,
isTopLevel=isTopLevel@entry=true, run_once=run_once@entry=true,
dest=dest@entry=0x1b834b0, altdest=altdest@entry=0x1b834b0,
completionTag=0x7ffef6c8a3c0 "") at pquery.c:799
#17 0x00000000007a0f3b in exec_simple_query
(query_string=query_string@entry=0x1b82668 "REFRESH MATERIALIZED VIEW
CONCURRENTLY mvtest_tm;") at postgres.c:1122
#18 0x00000000007a2c88 in PostgresMain (argc=<optimized out>,
argv=argv@entry=0x1bad178, dbname=0x1bacfe0 "ddolgov",
username=<optimized out>) at postgres.c:4153
#19 0x0000000000720378 in BackendRun (port=port@entry=0x1ba3980) at
postmaster.c:4361
#20 0x0000000000722f65 in BackendStartup (port=port@entry=0x1ba3980)
at postmaster.c:4033
#21 0x0000000000723245 in ServerLoop () at postmaster.c:1706
#22 0x00000000007244ca in PostmasterMain (argc=argc@entry=3,
argv=argv@entry=0x1b7d0f0) at postmaster.c:1379
#23 0x0000000000689e58 in main (argc=3, argv=0x1b7d0f0) at main.c:228

Interesting enough that without jit_optimize_above_cost everything is fine, so
I assume there is a problem in the optimized bitcode. So far I'm investigating
what exactly is wrong, but just visually disassembled version of bitcode with
and without optimization look indeed quite different.

[1]: https://www.postgresql.org/message-id/CA%2Bq6zcXZRZHVpWQeKoM%2BoG%2B6-ApPH9VnFLNTUrXS6Uk%2BKD2twg%40mail.gmail.com

  2. Re: LLVM jit and matview

    Michael Paquier <michael@paquier.xyz> — 2018-07-10T00:19:58Z

    On Mon, Jul 09, 2018 at 04:04:11PM +0200, Dmitry Dolgov wrote:
> # matview.sql
> ...
> =# REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_tm;
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.

Problem reproduced here with exactly the same stack.  Here is a simple
SQL sequence:
create table aa (a int);
create materialized view aam as select * from aa;
 create unique index aami on aam(a);
insert into aa values (generate_series(1,100000));
refresh materialized view CONCURRENTLY aam;

I have added an open item.
--
Michael

  3. Re: LLVM jit and matview

    Andres Freund <andres@anarazel.de> — 2018-07-25T02:49:56Z

    On 2018-07-10 09:19:58 +0900, Michael Paquier wrote:
> On Mon, Jul 09, 2018 at 04:04:11PM +0200, Dmitry Dolgov wrote:
> > # matview.sql
> > ...
> > =# REFRESH MATERIALIZED VIEW CONCURRENTLY mvtest_tm;
> > server closed the connection unexpectedly
> > This probably means the server terminated abnormally
> > before or while processing the request.
> > The connection to the server was lost. Attempting reset: Failed.
> 
> Problem reproduced here with exactly the same stack.  Here is a simple
> SQL sequence:
> create table aa (a int);
> create materialized view aam as select * from aa;
>  create unique index aami on aam(a);
> insert into aa values (generate_series(1,100000));
> refresh materialized view CONCURRENTLY aam;

FWIW, this doesn't crash for me, using a trunk checkout for LLVM. I'll
try older ones, as soon as they finish rebuilding. But perhaps you could
re-verify that this still is an issue on recent PG checkouts? And which
version of LLVM are you guys using?

Greetings,

Andres Freund

  4. Re: LLVM jit and matview

    Michael Paquier <michael@paquier.xyz> — 2018-07-25T05:48:57Z

    On Tue, Jul 24, 2018 at 07:49:56PM -0700, Andres Freund wrote:
> FWIW, this doesn't crash for me, using a trunk checkout for LLVM. I'll
> try older ones, as soon as they finish rebuilding. But perhaps you could
> re-verify that this still is an issue on recent PG checkouts? And which
> version of LLVM are you guys using?

One of your recent fixes has visibly taken take of the issue (I am too
lazy to check which one).  I am using llvm 6.0 with Debian SID as far as
I know, configure links to stuff in /usr/lib/llvm-6.0/.
--
Michael

  5. Re: LLVM jit and matview

    Dmitry Dolgov <9erthalion6@gmail.com> — 2018-07-25T12:59:20Z

    > On Wed, 25 Jul 2018 at 07:49, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jul 24, 2018 at 07:49:56PM -0700, Andres Freund wrote:
> > FWIW, this doesn't crash for me, using a trunk checkout for LLVM. I'll
> > try older ones, as soon as they finish rebuilding. But perhaps you could
> > re-verify that this still is an issue on recent PG checkouts? And which
> > version of LLVM are you guys using?
>
> One of your recent fixes has visibly taken take of the issue (I am too
> lazy to check which one).  I am using llvm 6.0 with Debian SID as far as
> I know, configure links to stuff in /usr/lib/llvm-6.0/.

Looks like I still can reproduce it on the current head 167075be3ab. I was using
LLVM 5.0 package, but will try reproduce on the LLVM master branch as soon
as compilation will be finished (apparently, I need to rebuild it with
LLVM_USE_PERF, otherwise Postgres complains about undefined symbol:
LLVMCreatePerfJITEventListener).

  6. Re: LLVM jit and matview

    Andres Freund <andres@anarazel.de> — 2018-07-25T15:41:29Z

    On 2018-07-25 14:59:20 +0200, Dmitry Dolgov wrote:
> > On Wed, 25 Jul 2018 at 07:49, Michael Paquier <michael@paquier.xyz> wrote:
> >
> > On Tue, Jul 24, 2018 at 07:49:56PM -0700, Andres Freund wrote:
> > > FWIW, this doesn't crash for me, using a trunk checkout for LLVM. I'll
> > > try older ones, as soon as they finish rebuilding. But perhaps you could
> > > re-verify that this still is an issue on recent PG checkouts? And which
> > > version of LLVM are you guys using?
> >
> > One of your recent fixes has visibly taken take of the issue (I am too
> > lazy to check which one).  I am using llvm 6.0 with Debian SID as far as
> > I know, configure links to stuff in /usr/lib/llvm-6.0/.
> 
> Looks like I still can reproduce it on the current head 167075be3ab. I was using
> LLVM 5.0 package, but will try reproduce on the LLVM master branch as soon
> as compilation will be finished

Oh, interesting. It only crashes when compiling LLVM without LLVM's
asserts enabled, even when using exactly the same LLVM checkout for both
builds. No idea what that's about, yet.


> (apparently, I need to rebuild it with LLVM_USE_PERF, otherwise
> Postgres complains about undefined symbol:
> LLVMCreatePerfJITEventListener).

Sorry, that was an intermittent state, fixed now (was waiting for a
review to come in). OTOH, having perf support is good anyway ;)

Greetings,

Andres Freund

  7. Re: LLVM jit and matview

    Andres Freund <andres@anarazel.de> — 2018-07-25T21:47:02Z

    Hi,

On 2018-07-25 08:41:29 -0700, Andres Freund wrote:
> Oh, interesting. It only crashes when compiling LLVM without LLVM's
> asserts enabled, even when using exactly the same LLVM checkout for both
> builds. No idea what that's about, yet.

Oh, well, this took me longer than it should have.  The problem, to my
possibly demented mind, is actually kinda funny:

The problematic expression in the query invokes ExecEvalRowNull() (it's
EEOP_WHOLEROW followed by EEOP_NULLTEST_ROWISNOTNULL). With inlining
enabled, that ends up inlining ExecEvalRowNull(), ExecEvalRowNullInt(),
get_cached_rowtype(), ShutdownTupleDescRef() (largely because these are
static functions that'd otherwise prevent ExecEvalRowNull from being
inlined).
get_cached_rowtype() does
            /* Need to register shutdown callback to release tupdesc */
            RegisterExprContextCallback(econtext,
                                        ShutdownTupleDescRef,
                                        PointerGetDatum(cache_field));
which, in the inlined version, means that ShutdownTupleDescRef is the
inlined copy. So, the query execution sets up a shutdown callback, that
is a JITed function.

But note standard_ExecutorEnd():

	/* release JIT context, if allocated */
	if (estate->es_jit)
	{
		jit_release_context(estate->es_jit);
		estate->es_jit = NULL;
	}

	/*
	 * Must switch out of context before destroying it
	 */
	MemoryContextSwitchTo(oldcontext);

	/*
	 * Release EState and per-query memory context.  This should release
	 * everything the executor has allocated.
	 */
	FreeExecutorState(estate);

FreeExecutorState(), which calls the shutdown callbacks, is executed
*after* the JIT context is destroyed! Thereby causing the segfault, as
the shutdown callback now invokes unmapped memory.

The fix is easy, releasing the JIT context should just happen in
FreeExecutorState(). Only thing is that that function has the following
comment in the header:
 * Note: this is not responsible for releasing non-memory resources,
 * such as open relations or buffer pins.  But it will shut down any
 * still-active ExprContexts within the EState.  That is sufficient
 * cleanup for situations where the EState has only been used for expression
 * evaluation, and not to run a complete Plan.

I don't really think throwing away functions is a violation of that, but
I think it's possible to argue the other way?

Greetings,

Andres Freund

  8. Re: LLVM jit and matview

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-25T22:11:13Z

    On 2018-Jul-25, Andres Freund wrote:

> The fix is easy, releasing the JIT context should just happen in
> FreeExecutorState(). Only thing is that that function has the following
> comment in the header:
>  * Note: this is not responsible for releasing non-memory resources,
>  * such as open relations or buffer pins.  But it will shut down any
>  * still-active ExprContexts within the EState.  That is sufficient
>  * cleanup for situations where the EState has only been used for expression
>  * evaluation, and not to run a complete Plan.
> 
> I don't really think throwing away functions is a violation of that, but
> I think it's possible to argue the other way?

I suppose the other possible way about it is to say estate->es_jit in a
local variable so that you can call it after FreeExecutorState.  But
what would be the advantage of avoiding the context release inside
FreeExecutorState?  It seems pretty appropriate to me to do it there.
You could argue that the JIT context is definitely part of the estate
being freed.  Just amend the comment, no?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  9. Re: LLVM jit and matview

    Andres Freund <andres@anarazel.de> — 2018-07-25T22:13:58Z

    On 2018-07-25 18:11:13 -0400, Alvaro Herrera wrote:
> On 2018-Jul-25, Andres Freund wrote:
> 
> > The fix is easy, releasing the JIT context should just happen in
> > FreeExecutorState(). Only thing is that that function has the following
> > comment in the header:
> >  * Note: this is not responsible for releasing non-memory resources,
> >  * such as open relations or buffer pins.  But it will shut down any
> >  * still-active ExprContexts within the EState.  That is sufficient
> >  * cleanup for situations where the EState has only been used for expression
> >  * evaluation, and not to run a complete Plan.
> > 
> > I don't really think throwing away functions is a violation of that, but
> > I think it's possible to argue the other way?
> 
> I suppose the other possible way about it is to say estate->es_jit in a
> local variable so that you can call it after FreeExecutorState.

That doesn't work, as the object pointed to by estate->es_jit is also
allocated inside the context that estate->es_jit deletes.

> But what would be the advantage of avoiding the context release inside
> FreeExecutorState?  It seems pretty appropriate to me to do it there.
> You could argue that the JIT context is definitely part of the estate
> being freed.  Just amend the comment, no?

I agree it's right to do it there.  I think I'm more questioning whether
there's even a need to adapt the comment, given it's really a local
memory resource. But I guess I'll just add a 'and ...' after
"ExprContexts within the EState".

Greetings,

Andres Freund

  10. Re: LLVM jit and matview

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-07-25T22:39:38Z

    On 2018-Jul-25, Andres Freund wrote:

> On 2018-07-25 18:11:13 -0400, Alvaro Herrera wrote:
> > On 2018-Jul-25, Andres Freund wrote:

> > But what would be the advantage of avoiding the context release inside
> > FreeExecutorState?  It seems pretty appropriate to me to do it there.
> > You could argue that the JIT context is definitely part of the estate
> > being freed.  Just amend the comment, no?
> 
> I agree it's right to do it there.

Ok.

> I think I'm more questioning whether there's even a need to adapt the
> comment, given it's really a local memory resource. But I guess I'll
> just add a 'and ...' after "ExprContexts within the EState".

Yeah, adding two words there sounds good.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: LLVM jit and matview

    Tom Lane <tgl@sss.pgh.pa.us> — 2018-07-25T22:59:51Z

    Andres Freund <andres@anarazel.de> writes:
> On 2018-07-25 18:11:13 -0400, Alvaro Herrera wrote:
>> But what would be the advantage of avoiding the context release inside
>> FreeExecutorState?  It seems pretty appropriate to me to do it there.
>> You could argue that the JIT context is definitely part of the estate
>> being freed.  Just amend the comment, no?

> I agree it's right to do it there.  I think I'm more questioning whether
> there's even a need to adapt the comment, given it's really a local
> memory resource. But I guess I'll just add a 'and ...' after
> "ExprContexts within the EState".

+1.  Clarity is good.

			regards, tom lane

  12. Re: LLVM jit and matview

    Andres Freund <andres@anarazel.de> — 2018-07-25T23:48:26Z

    On 2018-07-25 18:59:51 -0400, Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On 2018-07-25 18:11:13 -0400, Alvaro Herrera wrote:
> >> But what would be the advantage of avoiding the context release inside
> >> FreeExecutorState?  It seems pretty appropriate to me to do it there.
> >> You could argue that the JIT context is definitely part of the estate
> >> being freed.  Just amend the comment, no?
> 
> > I agree it's right to do it there.  I think I'm more questioning whether
> > there's even a need to adapt the comment, given it's really a local
> > memory resource. But I guess I'll just add a 'and ...' after
> > "ExprContexts within the EState".
> 
> +1.  Clarity is good.

Pushed something along those lines.

Thanks again Dmitry for reporting the issue!


- Andres