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
