From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: ExecAggTransReparent is underdocumented and badly named
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <3004180.1681930201.0@sss.pgh.pa.us>
Date: Wed, 19 Apr 2023 14:50:51 -0400
Message-ID: <3004282.1681930251@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/3004282.1681930251%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <3004180.1681930201.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="document-ExecAggTransReparent-better.patch";
	charset="us-ascii"
Content-ID: <3004180.1681930201.2@sss.pgh.pa.us>
Content-Description: document-ExecAggTransReparent-better.patch
Content-Transfer-Encoding: quoted-printable

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, AggStatePerTr=
ans 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 objec=
t
+ * that is already a child of the aggcontext.  What it will *not* do is w=
hat
+ * 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 d=
eals
+ * in expanded objects and is aware of the memory management conventions =
for
+ * aggregate transition values will have returned a R/W expanded object t=
hat
+ * is already in the right context.  However, if we have a generic transf=
n
+ * 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 tra=
nsfn,
+ * it will be at liberty to change or delete that object, and if it delet=
es 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 alway=
s
+ * 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, AggStateP=
erTrans 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 s=
et
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/n=
odeWindowAgg.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) !=3D DatumGetPointer(peraggstate->transValue))
@@ -533,7 +534,8 @@ advance_windowaggregate_base(WindowAggState *winstate,
 	 * free the prior transValue.  But if invtransfn returned a pointer to i=
ts
 	 * 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 o=
f
-	 * the aggcontext, assume we can adopt that value without copying it.
+	 * the aggcontext, assume we can adopt that value without copying it. (S=
ee
+	 * 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.

------- =_aaaaaaaaaa0--