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.
