Обсуждение: LLVM jit and matview
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
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
Вложения
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
Вложения
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
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
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
Вложения
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
Вложения
> 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).
> 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).
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
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
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
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
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
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
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
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
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
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
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
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
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
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