aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--src/backend/executor/execExprInterp.c49
-rw-r--r--src/backend/executor/nodeAgg.c14
-rw-r--r--src/backend/executor/nodeWindowAgg.c6
-rw-r--r--src/backend/jit/llvm/llvmjit_expr.c2
-rw-r--r--src/backend/jit/llvm/llvmjit_types.c2
-rw-r--r--src/include/executor/execExpr.h6
6 files changed, 51 insertions, 28 deletions
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 4cd46f17176..ca44d39100a 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4341,15 +4341,40 @@ ExecAggInitGroup(AggState *aggstate, AggStatePerTrans pertrans, AggStatePerGroup
}
/*
- * Ensure that the current transition value is a child of the aggcontext,
- * rather than the per-tuple context.
+ * Ensure that the new transition value is stored in the aggcontext,
+ * rather than the per-tuple context. This should be invoked only when
+ * we know (a) the transition data type is pass-by-reference, and (b)
+ * the newValue is distinct from the oldValue.
*
* NB: This can change the current memory context.
+ *
+ * We copy the presented newValue into the aggcontext, except when the datum
+ * points to a R/W expanded object that is already a child of the aggcontext,
+ * in which case we need not copy. We then delete the oldValue, if not null.
+ *
+ * If the presented datum points to a R/W expanded object that is a child of
+ * some other context, ideally we would just reparent it under the aggcontext.
+ * Unfortunately, that doesn't work easily, and it wouldn't help anyway for
+ * aggregate-aware transfns. We expect that a transfn that deals in expanded
+ * objects and is aware of the memory management conventions for aggregate
+ * transition values will (1) on first call, return a R/W expanded object that
+ * is already in the right context, allowing us to do nothing here, and (2) on
+ * subsequent calls, modify and return that same object, so that control
+ * doesn't even reach here. However, if we have a generic transfn that
+ * returns a new R/W expanded object (probably in the per-tuple context),
+ * reparenting that result would cause problems. We'd pass that R/W object to
+ * the next invocation of the transfn, and then it would be at liberty to
+ * change or delete that object, and if it deletes it then our own attempt to
+ * delete the now-old transvalue afterwards would be a double free. We avoid
+ * this problem by forcing the stored transvalue to always be a flat
+ * non-expanded object unless the transfn is visibly doing aggregate-aware
+ * memory management. This is somewhat inefficient, but the best answer to
+ * that is to write a smarter transfn.
*/
Datum
-ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
- Datum newValue, bool newValueIsNull,
- Datum oldValue, bool oldValueIsNull)
+ExecAggCopyTransValue(AggState *aggstate, AggStatePerTrans pertrans,
+ Datum newValue, bool newValueIsNull,
+ Datum oldValue, bool oldValueIsNull)
{
Assert(newValue != oldValue);
@@ -4559,12 +4584,10 @@ ExecAggPlainTransByRef(AggState *aggstate, AggStatePerTrans pertrans,
/*
* For pass-by-ref datatype, must copy the new value into aggcontext and
* free the prior transValue. But if transfn returned a pointer to its
- * 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.
+ * first input, we don't need to do anything.
*
* It's safe to compare newVal with pergroup->transValue without regard
- * for either being NULL, because ExecAggTransReparent() takes care to set
+ * for either being NULL, because ExecAggCopyTransValue 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
@@ -4573,10 +4596,10 @@ ExecAggPlainTransByRef(AggState *aggstate, AggStatePerTrans pertrans,
* argument.
*/
if (DatumGetPointer(newVal) != DatumGetPointer(pergroup->transValue))
- newVal = ExecAggTransReparent(aggstate, pertrans,
- newVal, fcinfo->isnull,
- pergroup->transValue,
- pergroup->transValueIsNull);
+ newVal = ExecAggCopyTransValue(aggstate, pertrans,
+ newVal, fcinfo->isnull,
+ pergroup->transValue,
+ pergroup->transValueIsNull);
pergroup->transValue = newVal;
pergroup->transValueIsNull = fcinfo->isnull;
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index 3aab5a0e80b..cfadef09420 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/src/backend/executor/nodeAgg.c
@@ -778,12 +778,10 @@ advance_transition_function(AggState *aggstate,
/*
* If pass-by-ref datatype, must copy the new value into aggcontext and
* free the prior transValue. But if transfn returned a pointer to its
- * 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.
+ * first input, we don't need to do anything.
*
* It's safe to compare newVal with pergroup->transValue without regard
- * for either being NULL, because ExecAggTransReparent() takes care to set
+ * for either being NULL, because ExecAggCopyTransValue 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
@@ -793,10 +791,10 @@ advance_transition_function(AggState *aggstate,
*/
if (!pertrans->transtypeByVal &&
DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue))
- newVal = ExecAggTransReparent(aggstate, pertrans,
- newVal, fcinfo->isnull,
- pergroupstate->transValue,
- pergroupstate->transValueIsNull);
+ newVal = ExecAggCopyTransValue(aggstate, pertrans,
+ newVal, fcinfo->isnull,
+ pergroupstate->transValue,
+ pergroupstate->transValueIsNull);
pergroupstate->transValue = newVal;
pergroupstate->transValueIsNull = fcinfo->isnull;
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 3ac581a7113..4f0618f27ab 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -368,7 +368,8 @@ advance_windowaggregate(WindowAggState *winstate,
* free the prior transValue. But if transfn returned a pointer to its
* 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.
+ * aggcontext, assume we can adopt that value without copying it. (See
+ * comments for ExecAggCopyTransValue, which this code duplicates.)
*/
if (!peraggstate->transtypeByVal &&
DatumGetPointer(newVal) != DatumGetPointer(peraggstate->transValue))
@@ -533,7 +534,8 @@ advance_windowaggregate_base(WindowAggState *winstate,
* free the prior transValue. But if invtransfn returned a pointer to its
* first input, we don't need to do anything. Also, if invtransfn
* 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.
+ * the aggcontext, assume we can adopt that value without copying it. (See
+ * comments for ExecAggCopyTransValue, which this code duplicates.)
*
* Note: the checks for null values here will never fire, but it seems
* best to have this stanza look just like advance_windowaggregate.
diff --git a/src/backend/jit/llvm/llvmjit_expr.c b/src/backend/jit/llvm/llvmjit_expr.c
index daefe66f40a..a1b33d6e6cf 100644
--- a/src/backend/jit/llvm/llvmjit_expr.c
+++ b/src/backend/jit/llvm/llvmjit_expr.c
@@ -2314,7 +2314,7 @@ llvm_compile_expr(ExprState *state)
params[5] = LLVMBuildTrunc(b, v_transnull,
TypeParamBool, "");
- v_fn = llvm_pg_func(mod, "ExecAggTransReparent");
+ v_fn = llvm_pg_func(mod, "ExecAggCopyTransValue");
v_newval =
LLVMBuildCall(b, v_fn,
params, lengthof(params),
diff --git a/src/backend/jit/llvm/llvmjit_types.c b/src/backend/jit/llvm/llvmjit_types.c
index f61d9390ee8..feb8208b797 100644
--- a/src/backend/jit/llvm/llvmjit_types.c
+++ b/src/backend/jit/llvm/llvmjit_types.c
@@ -102,7 +102,7 @@ FunctionReturningBool(void)
void *referenced_functions[] =
{
ExecAggInitGroup,
- ExecAggTransReparent,
+ ExecAggCopyTransValue,
ExecEvalPreOrderedDistinctSingle,
ExecEvalPreOrderedDistinctMulti,
ExecEvalAggOrderedTransDatum,
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index ea3ac10876f..157b0d85f29 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -807,9 +807,9 @@ extern void ExecEvalSysVar(ExprState *state, ExprEvalStep *op,
extern void ExecAggInitGroup(AggState *aggstate, AggStatePerTrans pertrans, AggStatePerGroup pergroup,
ExprContext *aggcontext);
-extern Datum ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
- Datum newValue, bool newValueIsNull,
- Datum oldValue, bool oldValueIsNull);
+extern Datum ExecAggCopyTransValue(AggState *aggstate, AggStatePerTrans pertrans,
+ Datum newValue, bool newValueIsNull,
+ Datum oldValue, bool oldValueIsNull);
extern bool ExecEvalPreOrderedDistinctSingle(AggState *aggstate,
AggStatePerTrans pertrans);
extern bool ExecEvalPreOrderedDistinctMulti(AggState *aggstate,
generated by cgit v1.2.3 (git 2.25.1) at 2025-05-19 17:11:12 +0000