Remove uses of MemoryContextContains in nodeAgg.c and nodeWindowAgg.c.

MemoryContextContains is no longer reliable in the wake of c6e0fe1f2,
so we need to get rid of these uses.

It appears that there's no really good reason to force the result of
an aggregate's finalfn or serialfn to be allocated in the per-tuple
context.  The only other plausible case is that the result points to
or into the aggregate's transition value, and that's fine because it
will last as long as we need it to.  (This conclusion depends on the
assumption that finalfns are not allowed to scribble on the transition
value, but we've long required that.)  So we can just drop the
MemoryContextContains plus datumCopy business, although we do need
to take care to not return a read-write pointer when the transition
value is an expanded datum.

Likewise, we don't really need to force the result of a window
function to be in the output context.  In this case, the plausible
alternative is that it's pointing into the temporary tuple slot used
by WinGetFuncArgInPartition or WinGetFuncArgInFrame (since those
functions could return such a pointer, which might become the window
function's result).  That will hold still for long enough, unless
there is another window function using the same WindowObject.
I'm content to always perform a datumCopy when there's more than one
such function.

On net, these changes should provide small speed improvements as well
as removing problematic code.

Tom Lane and David Rowley

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

src/backend/executor/nodeAgg.c

@@ -1028,6 +1028,8 @@ process_ordered_aggregate_multi(AggState *aggstate,
  *
  * The finalfn will be run, and the result delivered, in the
  * output-tuple context; caller's CurrentMemoryContext does not matter.
+ * (But note that in some cases, such as when there is no finalfn, the
+ * result might be a pointer to or into the agg's transition value.)
  *
  * The finalfn uses the state as set in the transno. This also might be
  * being used by another aggregate function, so it's important that we do
@@ -1112,21 +1114,13 @@ finalize_aggregate(AggState *aggstate,
 	}
 	else
 	{
-		/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
-		*resultVal = pergroupstate->transValue;
+		*resultVal =
+			MakeExpandedObjectReadOnly(pergroupstate->transValue,
+									   pergroupstate->transValueIsNull,
+									   pertrans->transtypeLen);
 		*resultIsNull = pergroupstate->transValueIsNull;
 	}
 
-	/*
-	 * If result is pass-by-ref, make sure it is in the right context.
-	 */
-	if (!peragg->resulttypeByVal && !*resultIsNull &&
-		!MemoryContextContains(CurrentMemoryContext,
-							   DatumGetPointer(*resultVal)))
-		*resultVal = datumCopy(*resultVal,
-							   peragg->resulttypeByVal,
-							   peragg->resulttypeLen);
-
 	MemoryContextSwitchTo(oldContext);
 }
 
@@ -1176,19 +1170,13 @@ finalize_partialaggregate(AggState *aggstate,
 	}
 	else
 	{
-		/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
-		*resultVal = pergroupstate->transValue;
+		*resultVal =
+			MakeExpandedObjectReadOnly(pergroupstate->transValue,
+									   pergroupstate->transValueIsNull,
+									   pertrans->transtypeLen);
 		*resultIsNull = pergroupstate->transValueIsNull;
 	}
 
-	/* If result is pass-by-ref, make sure it is in the right context. */
-	if (!peragg->resulttypeByVal && !*resultIsNull &&
-		!MemoryContextContains(CurrentMemoryContext,
-							   DatumGetPointer(*resultVal)))
-		*resultVal = datumCopy(*resultVal,
-							   peragg->resulttypeByVal,
-							   peragg->resulttypeLen);
-
 	MemoryContextSwitchTo(oldContext);
 }
 

src/backend/executor/nodeWindowAgg.c

@@ -630,20 +630,13 @@ finalize_windowaggregate(WindowAggState *winstate,
 	}
 	else
 	{
-		/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
-		*result = peraggstate->transValue;
+		*result =
+			MakeExpandedObjectReadOnly(peraggstate->transValue,
+									   peraggstate->transValueIsNull,
+									   peraggstate->transtypeLen);
 		*isnull = peraggstate->transValueIsNull;
 	}
 
-	/*
-	 * If result is pass-by-ref, make sure it is in the right context.
-	 */
-	if (!peraggstate->resulttypeByVal && !*isnull &&
-		!MemoryContextContains(CurrentMemoryContext,
-							   DatumGetPointer(*result)))
-		*result = datumCopy(*result,
-							peraggstate->resulttypeByVal,
-							peraggstate->resulttypeLen);
 	MemoryContextSwitchTo(oldContext);
 }
 
@@ -1057,13 +1050,14 @@ eval_windowfunction(WindowAggState *winstate, WindowStatePerFunc perfuncstate,
 	*isnull = fcinfo->isnull;
 
 	/*
-	 * Make sure pass-by-ref data is allocated in the appropriate context. (We
-	 * need this in case the function returns a pointer into some short-lived
-	 * tuple, as is entirely possible.)
+	 * The window function might have returned a pass-by-ref result that's
+	 * just a pointer into one of the WindowObject's temporary slots.  That's
+	 * not a problem if it's the only window function using the WindowObject;
+	 * but if there's more than one function, we'd better copy the result to
+	 * ensure it's not clobbered by later window functions.
 	 */
 	if (!perfuncstate->resulttypeByVal && !fcinfo->isnull &&
-		!MemoryContextContains(CurrentMemoryContext,
-							   DatumGetPointer(*result)))
+		winstate->numfuncs > 1)
 		*result = datumCopy(*result,
 							perfuncstate->resulttypeByVal,
 							perfuncstate->resulttypeLen);
@@ -3111,6 +3105,10 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
 	/*
 	 * Now we should be on the tuple immediately before or after the one we
 	 * want, so just fetch forwards or backwards as appropriate.
+	 *
+	 * Notice that we tell tuplestore_gettupleslot to make a physical copy of
+	 * the fetched tuple.  This ensures that the slot's contents remain valid
+	 * through manipulations of the tuplestore, which some callers depend on.
 	 */
 	if (winobj->seekpos > pos)
 	{

src/test/regress/expected/window.out

@@ -657,6 +657,23 @@ select first_value(max(x)) over (), y
          ->  Seq Scan on tenk1
 (4 rows)
 
+-- window functions returning pass-by-ref values from different rows
+select x, lag(x, 1) over (order by x), lead(x, 3) over (order by x)
+from (select x::numeric as x from generate_series(1,10) x);
+ x  | lag | lead 
+----+-----+------
+  1 |     |    4
+  2 |   1 |    5
+  3 |   2 |    6
+  4 |   3 |    7
+  5 |   4 |    8
+  6 |   5 |    9
+  7 |   6 |   10
+  8 |   7 |     
+  9 |   8 |     
+ 10 |   9 |     
+(10 rows)
+
 -- test non-default frame specifications
 SELECT four, ten,
 	sum(ten) over (partition by four order by ten),

src/test/regress/sql/window.sql

@@ -153,6 +153,10 @@ select first_value(max(x)) over (), y
   from (select unique1 as x, ten+four as y from tenk1) ss
   group by y;
 
+-- window functions returning pass-by-ref values from different rows
+select x, lag(x, 1) over (order by x), lead(x, 3) over (order by x)
+from (select x::numeric as x from generate_series(1,10) x);
+
 -- test non-default frame specifications
 SELECT four, ten,
 	sum(ten) over (partition by four order by ten),