Rename ExecAggTransReparent, and improve its documentation.

The name of this function suggests that it ought to reparent R/W
expanded objects to be children of the persistent aggcontext, instead
of copying them.  In fact it does no such thing, and if you try to
make it do so you will see multiple regression failures.  Rename it
to the less-misleading ExecAggCopyTransValue, and add commentary
about why that attractive-sounding optimization won't work.  Also
adjust comments at call sites, some of which were describing logic
that has since been moved into ExecAggCopyTransValue.

Discussion: https://postgr.es/m/3004282.1681930251@sss.pgh.pa.us

diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 4cd46f1717653d96b94997bc9741f80f053917f0..ca44d39100a32ecbfeaba4f4f3e1a210be441e87 100644 (file)
--- 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 3aab5a0e80bba3043853902f695377c41fd6dc42..cfadef09420bbad84b9c1c2ccd289417718134e1 100644 (file)
--- 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 3ac581a71137aaca879598c5ea3b55d56a725b00..4f0618f27ab51b3fc2610e555b18f88e53340ba8 100644 (file)
--- 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 daefe66f40a644f893bcc12da93910e95d870567..a1b33d6e6cf9fc651ee164561d32413a281d7b2c 100644 (file)
--- 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 f61d9390ee8bd21631c4396e37e64b9accc1f990..feb8208b7975889ea25ffe8295078a921616bd51 100644 (file)
--- 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 ea3ac10876faffaa572859a3515c55bf31b406c4..157b0d85f29286166e1edb495f2bba46a47f81ce 100644 (file)
--- 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,