diff options
| -rw-r--r-- | src/backend/executor/nodeAgg.c | 32 | ||||
| -rw-r--r-- | src/backend/executor/nodeWindowAgg.c | 30 | ||||
| -rw-r--r-- | src/test/regress/expected/window.out | 17 | ||||
| -rw-r--r-- | 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 373bcf61889..bcf3b1950b1 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 8b0858e9f5f..1750121c492 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 55dcd668c94..170bea23c28 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 57c39e796c1..1138453131e 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), |