From affdde2e15d9df6e9736bbb7e7cd9d56049d2f5a Mon Sep 17 00:00:00 2001 From: Andres Freund Date: Mon, 20 Jan 2020 23:26:51 -0800 Subject: [PATCH] Fix edge case leading to agg transitions skipping ExecAggTransReparent() calls. The code checking whether an aggregate transition value needs to be reparented into the current context has always only compared the transition return value with the previous transition value by datum, i.e. without regard for NULLness. This normally works, because when the transition function returns NULL (via fcinfo->isnull), it'll return a value that won't be the same as its input value. But there's no hard requirement that that's the case. And it turns out, it's possible to hit this case (see discussion or reproducers), leading to a non-null transition value not being reparented, followed by a crash caused by that. Instead of adding another comparison of NULLness, instead have ExecAggTransReparent() ensure that pergroup->transValue ends up as 0 when the new transition value is NULL. That avoids having to add an additional branch to the much more common cases of the transition function returning the old transition value (which is a pointer in this case), and when the new value is different, but not NULL. In branches since 69c3936a149, also deduplicate the reparenting code between the expression evaluation based transitions, and the path for ordered aggregates. Reported-By: Teodor Sigaev, Nikita Glukhov Author: Andres Freund Discussion: https://postgr.es/m/bd34e930-cfec-ea9b-3827-a8bc50891393@sigaev.ru Backpatch: 9.4-, this issue has existed since at least 7.4 --- src/backend/executor/execExprInterp.c | 21 +++++++++++++++ src/backend/executor/nodeAgg.c | 38 ++++++++++----------------- 2 files changed, 35 insertions(+), 24 deletions(-) diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c index 790380051b..f901baf1ed 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -1742,6 +1742,15 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull) * anything. Also, if transfn returned a pointer to a R/W * expanded object that is already a child of the aggcontext, * assume we can adopt that value without copying it. + * + * It's safe to compare newVal with pergroup->transValue without + * regard for either being NULL, because ExecAggTransReparent() + * takes care to set transValue to 0 when NULL. Otherwise we could + * end up accidentally not reparenting, when the transValue has + * the same numerical value as newValue, despite being NULL. This + * is a somewhat hot path, making it undesirable to instead solve + * this with another branch for the common case of the transition + * function returning its (modified) input argument. */ if (DatumGetPointer(newVal) != DatumGetPointer(pergroup->transValue)) newVal = ExecAggTransReparent(aggstate, pertrans, @@ -4186,6 +4195,8 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans, Datum newValue, bool newValueIsNull, Datum oldValue, bool oldValueIsNull) { + Assert(newValue != oldValue); + if (!newValueIsNull) { MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory); @@ -4199,6 +4210,16 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans, pertrans->transtypeByVal, pertrans->transtypeLen); } + else + { + /* + * Ensure that AggStatePerGroup->transValue ends up being 0, so + * callers can safely compare newValue/oldValue without having to + * check their respective nullness. + */ + newValue = (Datum) 0; + } + if (!oldValueIsNull) { if (DatumIsReadWriteExpandedObject(oldValue, diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 98bee4c5a6..7b8cb91f04 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -221,6 +221,7 @@ #include "catalog/pg_aggregate.h" #include "catalog/pg_proc.h" #include "catalog/pg_type.h" +#include "executor/execExpr.h" #include "executor/executor.h" #include "executor/nodeAgg.h" #include "miscadmin.h" @@ -626,33 +627,22 @@ advance_transition_function(AggState *aggstate, * first input, we don't need to do anything. Also, if transfn returned a * pointer to a R/W expanded object that is already a child of the * aggcontext, assume we can adopt that value without copying it. + * + * It's safe to compare newVal with pergroup->transValue without + * regard for either being NULL, because ExecAggTransReparent() + * takes care to set transValue to 0 when NULL. Otherwise we could + * end up accidentally not reparenting, when the transValue has + * the same numerical value as newValue, despite being NULL. This + * is a somewhat hot path, making it undesirable to instead solve + * this with another branch for the common case of the transition + * function returning its (modified) input argument. */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) - { - if (!fcinfo->isnull) - { - MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory); - if (DatumIsReadWriteExpandedObject(newVal, - false, - pertrans->transtypeLen) && - MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context) == CurrentMemoryContext) - /* do nothing */ ; - else - newVal = datumCopy(newVal, - pertrans->transtypeByVal, - pertrans->transtypeLen); - } - if (!pergroupstate->transValueIsNull) - { - if (DatumIsReadWriteExpandedObject(pergroupstate->transValue, - false, - pertrans->transtypeLen)) - DeleteExpandedObject(pergroupstate->transValue); - else - pfree(DatumGetPointer(pergroupstate->transValue)); - } - } + newVal = ExecAggTransReparent(aggstate, pertrans, + newVal, fcinfo->isnull, + pergroupstate->transValue, + pergroupstate->transValueIsNull); pergroupstate->transValue = newVal; pergroupstate->transValueIsNull = fcinfo->isnull;