I wrote:
> I think what we should look at is extending the aggregate/window
> function APIs so that such functions can report where they put their
> output, and then we can nuke MemoryContextContains(), with the
> code code set up to assume that it has to copy if the called function
> didn't report anything. The existing FunctionCallInfo.resultinfo
> mechanism (cf. ReturnSetInfo for SRFs) is probably the right thing
> to pass the flag through.
After studying the existing usages of MemoryContextContains, I think
there is a better answer, which is to just nuke them.
As far as I can tell, the datumCopy steps associated with aggregate
finalfns are basically useless. They only serve to prevent
returning a pointer directly to the aggregate's transition value
(or, perhaps, to a portion thereof). But what's wrong with that?
It'll last as long as we need it to. Maybe there was a reason
back before we required finalfns to not scribble on the transition
values, but those days are gone.
The same goes for aggregate serialfns --- although there, I can't
avoid the feeling that the datumCopy step was just cargo-culted in.
I don't think there can exist a serialfn that doesn't return a
freshly-palloced bytea.
The one place where we actually need the conditional datumCopy is
with window functions, and even there I don't think we need it
in simple cases with only one window function. The case that is
hazardous is where multiple window functions are sharing a
WindowObject. So I'm content to optimize the single-window-function
case and just always copy if there's more than one. (Sadly, there
is no existing regression test that catches this, so I added one.)
See attached.
regards, tom lane
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c
index fe74e49814..45fcd37253 100644
--- a/src/backend/executor/nodeAgg.c
+++ b/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);
}
diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 8b0858e9f5..ce860bceeb 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/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);
diff --git a/src/test/regress/expected/window.out b/src/test/regress/expected/window.out
index 55dcd668c9..170bea23c2 100644
--- a/src/test/regress/expected/window.out
+++ b/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),
diff --git a/src/test/regress/sql/window.sql b/src/test/regress/sql/window.sql
index 57c39e796c..1138453131 100644
--- a/src/test/regress/sql/window.sql
+++ b/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),
diff --git a/src/backend/utils/mmgr/mcxt.c b/src/backend/utils/mmgr/mcxt.c
index 115a64cfe4..c80236d285 100644
--- a/src/backend/utils/mmgr/mcxt.c
+++ b/src/backend/utils/mmgr/mcxt.c
@@ -224,10 +224,8 @@ MemoryContextResetOnly(MemoryContext context)
* If context->ident points into the context's memory, it will become
* a dangling pointer. We could prevent that by setting it to NULL
* here, but that would break valid coding patterns that keep the
- * ident elsewhere, e.g. in a parent context. Another idea is to use
- * MemoryContextContains(), but we don't require ident strings to be
- * in separately-palloc'd chunks, so that risks false positives. So
- * for now we assume the programmer got it right.
+ * ident elsewhere, e.g. in a parent context. So for now we assume
+ * the programmer got it right.
*/
context->methods->reset(context);
@@ -482,15 +480,6 @@ MemoryContextAllowInCriticalSection(MemoryContext context, bool allow)
MemoryContext
GetMemoryChunkContext(void *pointer)
{
- /*
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- Assert(pointer != NULL);
- Assert(pointer == (void *) MAXALIGN(pointer));
- /* adding further Asserts here? See pre-checks in MemoryContextContains */
-
return MCXT_METHOD(pointer, get_chunk_context) (pointer);
}
@@ -813,49 +802,6 @@ MemoryContextCheck(MemoryContext context)
}
#endif
-/*
- * MemoryContextContains
- * Detect whether an allocated chunk of memory belongs to a given
- * context or not.
- *
- * Caution: 'pointer' must point to a pointer which was allocated by a
- * MemoryContext. It's not safe or valid to use this function on arbitrary
- * pointers as obtaining the MemoryContext which 'pointer' belongs to requires
- * possibly several pointer dereferences.
- */
-bool
-MemoryContextContains(MemoryContext context, void *pointer)
-{
- /*
- * Temporarily make this always return false as we don't yet have a fully
- * baked idea on how to make it work correctly with the new MemoryChunk
- * code.
- */
- return false;
-
-#ifdef NOT_USED
- MemoryContext ptr_context;
-
- /*
- * NB: We must perform run-time checks here which GetMemoryChunkContext()
- * does as assertions before calling GetMemoryChunkContext().
- *
- * Try to detect bogus pointers handed to us, poorly though we can.
- * Presumably, a pointer that isn't MAXALIGNED isn't pointing at an
- * allocated chunk.
- */
- if (pointer == NULL || pointer != (void *) MAXALIGN(pointer))
- return false;
-
- /*
- * OK, it's probably safe to look at the context.
- */
- ptr_context = GetMemoryChunkContext(pointer);
-
- return ptr_context == context;
-#endif
-}
-
/*
* MemoryContextCreate
* Context-type-independent part of context creation.
diff --git a/src/include/utils/memutils.h b/src/include/utils/memutils.h
index 52bc41ec53..4f6c5435ca 100644
--- a/src/include/utils/memutils.h
+++ b/src/include/utils/memutils.h
@@ -96,7 +96,6 @@ extern void MemoryContextAllowInCriticalSection(MemoryContext context,
#ifdef MEMORY_CONTEXT_CHECKING
extern void MemoryContextCheck(MemoryContext context);
#endif
-extern bool MemoryContextContains(MemoryContext context, void *pointer);
/* Handy macro for copying and assigning context ID ... but note double eval */
#define MemoryContextCopyAndSetIdentifier(cxt, id) \
