Re: LLVM jit and matview

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