ExecAggTransReparent is underdocumented and badly named - Mailing list pgsql-hackers

I wondered at [1] why ExecAggTransReparent doesn't do what you'd
guess from the name, that is reparent a supplied R/W expanded object
under the aggcontext even if it's not there already.  I tried to
make it do that, and after I'd finished bandaging my wounds,
I wrote some commentary explaining why that won't work.  (Possibly
we knew this at some point, but if so we sure failed to provide
documentation about it.)

I think we should at least apply the attached commentary-only
patch.  I wonder though if we should change the name of this
function, and if so to what.  Maybe ExecAggCopyTransValue?

            regards, tom lane

[1] https://www.postgresql.org/message-id/2199319.1681662388%40sss.pgh.pa.us

diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 4cd46f1717..386abf50b3 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -4341,10 +4341,27 @@ ExecAggInitGroup(AggState *aggstate, AggStatePerTrans pertrans, AggStatePerGroup
 }

 /*
- * Ensure that the current transition value is a child of the aggcontext,
+ * Ensure that the new transition value is stored in the aggcontext,
  * rather than the per-tuple context.
  *
  * NB: This can change the current memory context.
+ *
+ * This function is misnamed.  It can either copy the datum returned by
+ * the transfn, or do nothing if the datum points to a R/W expanded object
+ * that is already a child of the aggcontext.  What it will *not* do is what
+ * you might guess from the name, that is transfer a R/W expanded object from
+ * some other context to the aggcontext.  We expect that a transfn that deals
+ * in expanded objects and is aware of the memory management conventions for
+ * aggregate transition values will have returned a R/W expanded object that
+ * is already in the right context.  However, if we have a generic transfn
+ * that returns a R/W expanded object in the per-tuple context, we have a
+ * problem.  If we pass that R/W object to the next invocation of the transfn,
+ * it will 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 will be a
+ * double free.  We avoid this problem by forcing the 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,
@@ -4559,9 +4576,7 @@ 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
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 3ac581a711..75a4f83ff1 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 ExecAggTransReparent, 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 ExecAggTransReparent, 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.