diff options
| -rw-r--r-- | src/backend/executor/execExprInterp.c | 21 | ||||
| -rw-r--r-- | 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 66a67c72b29..fdb6ada6f31 100644 --- a/src/backend/executor/execExprInterp.c +++ b/src/backend/executor/execExprInterp.c @@ -1702,6 +1702,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, @@ -4087,6 +4096,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); @@ -4100,6 +4111,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 c4a5588113f..eef0e8437de 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; |