Thread: BUG #16112: large, unexpected memory consumption
The following bug has been logged on the website:
Bug reference: 16112
Logged by: Ben Cornett
Email address: ben@lantern.is
PostgreSQL version: 12.0
Operating system: linux 2.6.32
Description:
Creation of table t1 in the query below causes the server process to consume
close to 1GB of memory. The amount of memory consumed is proportional to
the value passed to generate_series in the first query.
CREATE temp TABLE t0 AS
SELECT
i,
ARRAY[1,2,3] a
FROM GENERATE_SERIES(1, 12000000) i
;
CREATE TEMP TABLE t1 AS
SELECT
i,
x
FROM t0, UNNEST(a) x;
I observed the same behavior in other test queries that included implicit
lateral joins.
Thanks very much,
Ben
On Wed, Nov 13, 2019 at 12:22:07PM +0000, PG Bug reporting form wrote: >The following bug has been logged on the website: > >Bug reference: 16112 >Logged by: Ben Cornett >Email address: ben@lantern.is >PostgreSQL version: 12.0 >Operating system: linux 2.6.32 >Description: > >Creation of table t1 in the query below causes the server process to consume >close to 1GB of memory. The amount of memory consumed is proportional to >the value passed to generate_series in the first query. > >CREATE temp TABLE t0 AS >SELECT > i, > ARRAY[1,2,3] a >FROM GENERATE_SERIES(1, 12000000) i >; > >CREATE TEMP TABLE t1 AS >SELECT > i, > x >FROM t0, UNNEST(a) x; > >I observed the same behavior in other test queries that included implicit >lateral joins. > Yeah, I can reproduce this pretty easily. It seems like a memory leak in ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be variable-length, but it gets allocated in ExecutorState context directly and so until the end of the query. The attached trivial patch fixes that by adding a pfree() at the end of the function. I wonder if we have the same issue elsewhere ... regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Hi, On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote: > Yeah, I can reproduce this pretty easily. It seems like a memory leak in > ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be > variable-length, but it gets allocated in ExecutorState context directly > and so until the end of the query. Damn. Too bad this got discovered just after the point release was wrapped :( > The attached trivial patch fixes that by adding a pfree() at the end of > the function. Hm. That's clearly an improvement. But I'm not quite sure it's really the right direction. It seems like a bad idea to rely on ExecMakeTableFunctionResult() otherwise never leaking any memory. It seems to we should be using memory contexts to provide a backstop against leaks, like we normally do, instead of operating in the per-query context. It's noteworthy that nodeProjectSet already does so. In contrast to nodeProjectSet, I think we'd need an additional memory context however, as we eagerly evaluate ValuePerCall expressions - and we'd like to reset the context after each iteration. I think we also ought to use SetExprState->fcinfo, instead of allocating a separate allocation in ExecMakeTableFunctionResult(). But that's perhaps less important. > I wonder if we have the same issue elsewhere ... Quite possible - but I think in most cases we are using memory contexts to protect against leaks like this (which is more efficient than retail pfree'ing). Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote:
>> The attached trivial patch fixes that by adding a pfree() at the end of
>> the function.
> Hm. That's clearly an improvement. But I'm not quite sure it's really
> the right direction. It seems like a bad idea to rely on
> ExecMakeTableFunctionResult() otherwise never leaking any memory.
Considering that ExecMakeTableFunctionResult went from zero pallocs
to one, I don't see a strong reason why it should have bothered with
a private memory context before, nor do I think that's a good
response now.
regards, tom lane
On Wed, Nov 13, 2019 at 09:05:28AM -0800, Andres Freund wrote: >Hi, > >On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote: >> Yeah, I can reproduce this pretty easily. It seems like a memory leak in >> ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be >> variable-length, but it gets allocated in ExecutorState context directly >> and so until the end of the query. > >Damn. Too bad this got discovered just after the point release was >wrapped :( > > >> The attached trivial patch fixes that by adding a pfree() at the end of >> the function. > >Hm. That's clearly an improvement. But I'm not quite sure it's really >the right direction. It seems like a bad idea to rely on >ExecMakeTableFunctionResult() otherwise never leaking any memory. > >It seems to we should be using memory contexts to provide a backstop >against leaks, like we normally do, instead of operating in the >per-query context. It's noteworthy that nodeProjectSet already does so. >In contrast to nodeProjectSet, I think we'd need an additional memory >context however, as we eagerly evaluate ValuePerCall expressions - and >we'd like to reset the context after each iteration. > Possibly, but I think most of the allocations already use the per-tuple context. It's just the fcinfo that seems to be allocated outside of it, so maybe we should just move it to that memory context. > >I think we also ought to use SetExprState->fcinfo, instead of allocating >a separate allocation in ExecMakeTableFunctionResult(). But that's >perhaps less important. > Maybe. > >> I wonder if we have the same issue elsewhere ... > >Quite possible - but I think in most cases we are using memory contexts >to protect against leaks like this (which is more efficient than retail >pfree'ing). > Yeah, probably. I had a quick look at some of those places (found by looking for SizeForFunctionCallInfo and palloc on the same line) and those that I looked at seemed fine. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-11-13 12:21:47 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > On 2019-11-13 15:50:04 +0100, Tomas Vondra wrote: > >> The attached trivial patch fixes that by adding a pfree() at the end of > >> the function. > > > Hm. That's clearly an improvement. But I'm not quite sure it's really > > the right direction. It seems like a bad idea to rely on > > ExecMakeTableFunctionResult() otherwise never leaking any memory. > > Considering that ExecMakeTableFunctionResult went from zero pallocs > to one, I don't see a strong reason why it should have bothered with > a private memory context before, nor do I think that's a good > response now. Well, that relies on type_is_rowtype(), exprType(), TupleDescInitEntry(), lookup_rowtype_tupdesc_copy() not to leak memory (besides lookup_rowtype_tupdesc_copy()'s return type, which is explicitly freed). Which is true, but still seems not super reliable. Thinking about it for a second longer, I don't think we'd need a new context - afaict argcontext has exactly the lifetime requirements needed. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Thinking about it for a second longer, I don't think we'd need a new
> context - afaict argcontext has exactly the lifetime requirements
> needed.
Hm, maybe so. That'd definitely be a better solution if it works.
regards, tom lane
On Wed, Nov 13, 2019 at 9:50 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> wrote:
Yeah, I can reproduce this pretty easily. It seems like a memory leak in
ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
variable-length, but it gets allocated in ExecutorState context directly
and so until the end of the query.
I find the leak was introduced much earlier than that, in:
commit 763f2edd92095b1ca2f4476da073a28505c13820
Author: Andres Freund <andres@anarazel.de>
Date: Thu Nov 15 14:26:14 2018 -0800
Rejigger materializing and fetching a HeapTuple from a slot.
Author: Andres Freund <andres@anarazel.de>
Date: Thu Nov 15 14:26:14 2018 -0800
Rejigger materializing and fetching a HeapTuple from a slot.
I have no idea if this info is useful to informing the best solution, though.
You patch applied to REL_12_STABLE does fix it for me.
The attached trivial patch fixes that by adding a pfree() at the end of
the function. I wonder if we have the same issue elsewhere ...
Is there an easy way to assess if the "make check" regression tests are taking more memory than they used to?
Cheers,
Jeff
Hi,
On 2019-11-13 17:53:28 -0500, Jeff Janes wrote:
> On Wed, Nov 13, 2019 at 9:50 AM Tomas Vondra <tomas.vondra@2ndquadrant.com>
> wrote:
>
> >
> > Yeah, I can reproduce this pretty easily. It seems like a memory leak in
> > ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be
> > variable-length, but it gets allocated in ExecutorState context directly
> > and so until the end of the query.
> >
>
> I find the leak was introduced much earlier than that, in:
>
> commit 763f2edd92095b1ca2f4476da073a28505c13820
> Author: Andres Freund <andres@anarazel.de>
> Date: Thu Nov 15 14:26:14 2018 -0800
>
> Rejigger materializing and fetching a HeapTuple from a slot.
>
> I have no idea if this info is useful to informing the best solution,
> though.
>
> You patch applied to REL_12_STABLE does fix it for me.
I think that's likely two overlapping issues.
Possibly
commit 88e6ad3054ddd5aa0dee12e5def2c335fe92a414
Author: Andres Freund <andres@anarazel.de>
Date: 2019-04-19 11:33:37 -0700
Fix two memory leaks around force-storing tuples in slots.
> > The attached trivial patch fixes that by adding a pfree() at the end of
> > the function. I wonder if we have the same issue elsewhere ...
> >
> >
> Is there an easy way to assess if the "make check" regression tests are
> taking more memory than they used to?
Not easily, I think. I also suspect that you'd not have seen a
meaningful increase in memory usage due to this bug - it "only" was
noticable for the query at hand, because the FunctionScan node was
reached so many times, due to the lateral join forcing it to be scanned
once for each value in the source temp table. I don't think we have a
query in the tests doing so often enough (partially because we don't,
partially because it'd take a lot of time on slow machines).
Greetings,
Andres Freund
On Wed, Nov 13, 2019 at 05:53:28PM -0500, Jeff Janes wrote: >On Wed, Nov 13, 2019 at 9:50 AM Tomas Vondra <tomas.vondra@2ndquadrant.com> >wrote: > >> >> Yeah, I can reproduce this pretty easily. It seems like a memory leak in >> ExecMakeTableFunctionResult. a9c35cf85ca reworked FunctionCallInfo to be >> variable-length, but it gets allocated in ExecutorState context directly >> and so until the end of the query. >> > >I find the leak was introduced much earlier than that, in: > >commit 763f2edd92095b1ca2f4476da073a28505c13820 >Author: Andres Freund <andres@anarazel.de> >Date: Thu Nov 15 14:26:14 2018 -0800 > > Rejigger materializing and fetching a HeapTuple from a slot. > >I have no idea if this info is useful to informing the best solution, >though. > Ah, I've only done a simple 'git blame' and assumed it's the last commit that touched the palloc line (and it seemed somewhat plausible). I don't think it changes the reasoning too much, though. >You patch applied to REL_12_STABLE does fix it for me. > > >> The attached trivial patch fixes that by adding a pfree() at the end of >> the function. I wonder if we have the same issue elsewhere ... >> >> >Is there an easy way to assess if the "make check" regression tests are >taking more memory than they used to? > Probably not. The regression tests use fairly small number of rows in general, and we don't have a way to track/inspect high watermaks or anything like that anyway. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2019-11-13 13:03:31 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Thinking about it for a second longer, I don't think we'd need a new > > context - afaict argcontext has exactly the lifetime requirements > > needed. > > Hm, maybe so. That'd definitely be a better solution if it works. Here's a patch doing so. I think it'd be a good idea to rename argcontext into something like setcontext or such. But as I'm not that happy with the name, I've not yet made that change. Greetings, Andres Freund
Attachment
Hi,
Did this patch ever get applied? It is not clear to me that it did.
Thanks!
On Thu, Nov 14, 2019 at 06:47:07PM -0800, Andres Freund wrote:
> Hi,
>
> On 2019-11-13 13:03:31 -0500, Tom Lane wrote:
> > Andres Freund <andres@anarazel.de> writes:
> > > Thinking about it for a second longer, I don't think we'd need a new
> > > context - afaict argcontext has exactly the lifetime requirements
> > > needed.
> >
> > Hm, maybe so. That'd definitely be a better solution if it works.
>
> Here's a patch doing so. I think it'd be a good idea to rename
> argcontext into something like setcontext or such. But as I'm not that
> happy with the name, I've not yet made that change.
>
> Greetings,
>
> Andres Freund
> diff --git i/src/backend/executor/execSRF.c w/src/backend/executor/execSRF.c
> index c8a3efc3654..85f33054265 100644
> --- i/src/backend/executor/execSRF.c
> +++ w/src/backend/executor/execSRF.c
> @@ -114,10 +114,23 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> ReturnSetInfo rsinfo;
> HeapTupleData tmptup;
> MemoryContext callerContext;
> - MemoryContext oldcontext;
> bool first_time = true;
>
> - callerContext = CurrentMemoryContext;
> + /*
> + * Execute per-tablefunc actions in appropriate context.
> + *
> + * The FunctionCallInfo needs to live across all the calls to a
> + * ValuePerCall function, so it can't be allocated in the per-tuple
> + * context. Similarly, the function arguments need to be evaluated in a
> + * context that is longer lived than the per-tuple context: The argument
> + * values would otherwise disappear when we reset that context in the
> + * inner loop. As the caller's CurrentMemoryContext is typically a
> + * query-lifespan context, we don't want to leak memory there. We require
> + * the caller to pass a separate memory context that can be used for this,
> + * and can be reset each time through to avoid bloat.
> + */
> + MemoryContextReset(argContext);
> + callerContext = MemoryContextSwitchTo(argContext);
>
> funcrettype = exprType((Node *) setexpr->expr);
>
> @@ -164,20 +177,9 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> setexpr->fcinfo->fncollation,
> NULL, (Node *) &rsinfo);
>
> - /*
> - * Evaluate the function's argument list.
> - *
> - * We can't do this in the per-tuple context: the argument values
> - * would disappear when we reset that context in the inner loop. And
> - * the caller's CurrentMemoryContext is typically a query-lifespan
> - * context, so we don't want to leak memory there. We require the
> - * caller to pass a separate memory context that can be used for this,
> - * and can be reset each time through to avoid bloat.
> - */
> - MemoryContextReset(argContext);
> - oldcontext = MemoryContextSwitchTo(argContext);
> + /* evaluate the function's argument list */
> + Assert(CurrentMemoryContext == argContext);
> ExecEvalFuncArgs(fcinfo, setexpr->args, econtext);
> - MemoryContextSwitchTo(oldcontext);
>
> /*
> * If function is strict, and there are any NULL arguments, skip
> @@ -217,7 +219,7 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> CHECK_FOR_INTERRUPTS();
>
> /*
> - * reset per-tuple memory context before each call of the function or
> + * Reset per-tuple memory context before each call of the function or
> * expression. This cleans up any local memory the function may leak
> * when called.
> */
> @@ -257,6 +259,8 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
> */
> if (first_time)
> {
> + MemoryContext oldcontext;
> +
> oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
> tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> rsinfo.setResult = tupstore;
> @@ -285,6 +289,8 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>
> if (tupdesc == NULL)
> {
> + MemoryContext oldcontext;
> +
> /*
> * This is the first non-NULL result from the
> * function. Use the type info embedded in the
> @@ -378,15 +384,18 @@ no_function_result:
> */
> if (rsinfo.setResult == NULL)
> {
> - MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
> + MemoryContext oldcontext;
> +
> + oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
> tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
> rsinfo.setResult = tupstore;
> + MemoryContextSwitchTo(oldcontext);
> +
> if (!returnsSet)
> {
> int natts = expectedDesc->natts;
> bool *nullflags;
>
> - MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
> nullflags = (bool *) palloc(natts * sizeof(bool));
> memset(nullflags, true, natts * sizeof(bool));
> tuplestore_putvalues(tupstore, expectedDesc, NULL, nullflags);
On Thu, Apr 09, 2020 at 08:21:54AM -0400, Ben Cornett wrote:
>Hi,
>
>Did this patch ever get applied? It is not clear to me that it did.
>
Hmmm, I don't think it got applied, which means we've missed yet another
minor release :-(
Andres, any plans to push this?
regards
>Thanks!
>
>On Thu, Nov 14, 2019 at 06:47:07PM -0800, Andres Freund wrote:
>> Hi,
>>
>> On 2019-11-13 13:03:31 -0500, Tom Lane wrote:
>> > Andres Freund <andres@anarazel.de> writes:
>> > > Thinking about it for a second longer, I don't think we'd need a new
>> > > context - afaict argcontext has exactly the lifetime requirements
>> > > needed.
>> >
>> > Hm, maybe so. That'd definitely be a better solution if it works.
>>
>> Here's a patch doing so. I think it'd be a good idea to rename
>> argcontext into something like setcontext or such. But as I'm not that
>> happy with the name, I've not yet made that change.
>>
>> Greetings,
>>
>> Andres Freund
>
>> diff --git i/src/backend/executor/execSRF.c w/src/backend/executor/execSRF.c
>> index c8a3efc3654..85f33054265 100644
>> --- i/src/backend/executor/execSRF.c
>> +++ w/src/backend/executor/execSRF.c
>> @@ -114,10 +114,23 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>> ReturnSetInfo rsinfo;
>> HeapTupleData tmptup;
>> MemoryContext callerContext;
>> - MemoryContext oldcontext;
>> bool first_time = true;
>>
>> - callerContext = CurrentMemoryContext;
>> + /*
>> + * Execute per-tablefunc actions in appropriate context.
>> + *
>> + * The FunctionCallInfo needs to live across all the calls to a
>> + * ValuePerCall function, so it can't be allocated in the per-tuple
>> + * context. Similarly, the function arguments need to be evaluated in a
>> + * context that is longer lived than the per-tuple context: The argument
>> + * values would otherwise disappear when we reset that context in the
>> + * inner loop. As the caller's CurrentMemoryContext is typically a
>> + * query-lifespan context, we don't want to leak memory there. We require
>> + * the caller to pass a separate memory context that can be used for this,
>> + * and can be reset each time through to avoid bloat.
>> + */
>> + MemoryContextReset(argContext);
>> + callerContext = MemoryContextSwitchTo(argContext);
>>
>> funcrettype = exprType((Node *) setexpr->expr);
>>
>> @@ -164,20 +177,9 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>> setexpr->fcinfo->fncollation,
>> NULL, (Node *) &rsinfo);
>>
>> - /*
>> - * Evaluate the function's argument list.
>> - *
>> - * We can't do this in the per-tuple context: the argument values
>> - * would disappear when we reset that context in the inner loop. And
>> - * the caller's CurrentMemoryContext is typically a query-lifespan
>> - * context, so we don't want to leak memory there. We require the
>> - * caller to pass a separate memory context that can be used for this,
>> - * and can be reset each time through to avoid bloat.
>> - */
>> - MemoryContextReset(argContext);
>> - oldcontext = MemoryContextSwitchTo(argContext);
>> + /* evaluate the function's argument list */
>> + Assert(CurrentMemoryContext == argContext);
>> ExecEvalFuncArgs(fcinfo, setexpr->args, econtext);
>> - MemoryContextSwitchTo(oldcontext);
>>
>> /*
>> * If function is strict, and there are any NULL arguments, skip
>> @@ -217,7 +219,7 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>> CHECK_FOR_INTERRUPTS();
>>
>> /*
>> - * reset per-tuple memory context before each call of the function or
>> + * Reset per-tuple memory context before each call of the function or
>> * expression. This cleans up any local memory the function may leak
>> * when called.
>> */
>> @@ -257,6 +259,8 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>> */
>> if (first_time)
>> {
>> + MemoryContext oldcontext;
>> +
>> oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
>> tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
>> rsinfo.setResult = tupstore;
>> @@ -285,6 +289,8 @@ ExecMakeTableFunctionResult(SetExprState *setexpr,
>>
>> if (tupdesc == NULL)
>> {
>> + MemoryContext oldcontext;
>> +
>> /*
>> * This is the first non-NULL result from the
>> * function. Use the type info embedded in the
>> @@ -378,15 +384,18 @@ no_function_result:
>> */
>> if (rsinfo.setResult == NULL)
>> {
>> - MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
>> + MemoryContext oldcontext;
>> +
>> + oldcontext = MemoryContextSwitchTo(econtext->ecxt_per_query_memory);
>> tupstore = tuplestore_begin_heap(randomAccess, false, work_mem);
>> rsinfo.setResult = tupstore;
>> + MemoryContextSwitchTo(oldcontext);
>> +
>> if (!returnsSet)
>> {
>> int natts = expectedDesc->natts;
>> bool *nullflags;
>>
>> - MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
>> nullflags = (bool *) palloc(natts * sizeof(bool));
>> memset(nullflags, true, natts * sizeof(bool));
>> tuplestore_putvalues(tupstore, expectedDesc, NULL, nullflags);
>
>
>
--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Hi, On 2020-04-11 20:35:52 +0200, Tomas Vondra wrote: > On Thu, Apr 09, 2020 at 08:21:54AM -0400, Ben Cornett wrote: Ben, for the future, it's better to not drop CC addresses etc. I didn't see this reply at the time you sent it, just when I was catching up on my backlog of unread postgres email. > > Did this patch ever get applied? It is not clear to me that it did. No :(. I lost track of it. I did recall that there was some patch I was supposed to get back to, but couldn't remember what exactly :(. It's really too bad that we don't have a bugtracker. I just can't keep track of all bugs / patches where I e.g. want to wait a few days for a reply (here to see whether Tom likes my alternative context based approach). > Hmmm, I don't think it got applied, which means we've missed yet another > minor release :-( > > Andres, any plans to push this? Yea, I think I'll go for my alternative approach from https://postgr.es/m/20191115024707.y5bcsb4oo4wzovu4%40alap3.anarazel.de Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes: > It's really too bad that we don't have a bugtracker. I just can't keep > track of all bugs / patches where I e.g. want to wait a few days for a > reply (here to see whether Tom likes my alternative context based > approach). > Yea, I think I'll go for my alternative approach from > https://postgr.es/m/20191115024707.y5bcsb4oo4wzovu4%40alap3.anarazel.de I haven't done any testing, but it looks reasonable. regards, tom lane
Hi, On 2020-04-20 15:19:13 -0400, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > It's really too bad that we don't have a bugtracker. I just can't keep > > track of all bugs / patches where I e.g. want to wait a few days for a > > reply (here to see whether Tom likes my alternative context based > > approach). > > > Yea, I think I'll go for my alternative approach from > > https://postgr.es/m/20191115024707.y5bcsb4oo4wzovu4%40alap3.anarazel.de > > I haven't done any testing, but it looks reasonable. Thanks for looking. Pushed. Ben, thanks for the report. And sorry that it took so long. Greetings, Andres Freund