From 42b746d4c982257bf3f924176632b04dc288174b Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 6 Oct 2022 13:27:34 -0400 Subject: [PATCH] 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 | 32 +++++++++------------------- src/backend/executor/nodeWindowAgg.c | 30 ++++++++++++-------------- src/test/regress/expected/window.out | 17 +++++++++++++++ src/test/regress/sql/window.sql | 4 ++++ 4 files changed, 45 insertions(+), 38 deletions(-) diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 373bcf6188..bcf3b1950b 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..1750121c49 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); @@ -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) { 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),