Re: Reducing the chunk header sizes on all memory context types - Mailing list pgsql-hackers

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) \