Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commit42b746d

Browse files
committed
Remove uses of MemoryContextContains in nodeAgg.c and nodeWindowAgg.c.
MemoryContextContains is no longer reliable in the wake ofc6e0fe1,so we need to get rid of these uses.It appears that there's no really good reason to force the result ofan aggregate's finalfn or serialfn to be allocated in the per-tuplecontext. The only other plausible case is that the result points toor into the aggregate's transition value, and that's fine because itwill last as long as we need it to. (This conclusion depends on theassumption that finalfns are not allowed to scribble on the transitionvalue, but we've long required that.) So we can just drop theMemoryContextContains plus datumCopy business, although we do needto take care to not return a read-write pointer when the transitionvalue is an expanded datum.Likewise, we don't really need to force the result of a windowfunction to be in the output context. In this case, the plausiblealternative is that it's pointing into the temporary tuple slot usedby WinGetFuncArgInPartition or WinGetFuncArgInFrame (since thosefunctions could return such a pointer, which might become the windowfunction's result). That will hold still for long enough, unlessthere is another window function using the same WindowObject.I'm content to always perform a datumCopy when there's more than onesuch function.On net, these changes should provide small speed improvements as wellas removing problematic code.Tom Lane and David RowleyDiscussion:https://postgr.es/m/1913788.1664898906@sss.pgh.pa.us
1 parent66c2922 commit42b746d

File tree

4 files changed

+45
-38
lines changed

4 files changed

+45
-38
lines changed

‎src/backend/executor/nodeAgg.c

Lines changed: 10 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -1028,6 +1028,8 @@ process_ordered_aggregate_multi(AggState *aggstate,
10281028
*
10291029
* The finalfn will be run, and the result delivered, in the
10301030
* output-tuple context; caller's CurrentMemoryContext does not matter.
1031+
* (But note that in some cases, such as when there is no finalfn, the
1032+
* result might be a pointer to or into the agg's transition value.)
10311033
*
10321034
* The finalfn uses the state as set in the transno. This also might be
10331035
* being used by another aggregate function, so it's important that we do
@@ -1112,21 +1114,13 @@ finalize_aggregate(AggState *aggstate,
11121114
}
11131115
else
11141116
{
1115-
/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
1116-
*resultVal=pergroupstate->transValue;
1117+
*resultVal=
1118+
MakeExpandedObjectReadOnly(pergroupstate->transValue,
1119+
pergroupstate->transValueIsNull,
1120+
pertrans->transtypeLen);
11171121
*resultIsNull=pergroupstate->transValueIsNull;
11181122
}
11191123

1120-
/*
1121-
* If result is pass-by-ref, make sure it is in the right context.
1122-
*/
1123-
if (!peragg->resulttypeByVal&& !*resultIsNull&&
1124-
!MemoryContextContains(CurrentMemoryContext,
1125-
DatumGetPointer(*resultVal)))
1126-
*resultVal=datumCopy(*resultVal,
1127-
peragg->resulttypeByVal,
1128-
peragg->resulttypeLen);
1129-
11301124
MemoryContextSwitchTo(oldContext);
11311125
}
11321126

@@ -1176,19 +1170,13 @@ finalize_partialaggregate(AggState *aggstate,
11761170
}
11771171
else
11781172
{
1179-
/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
1180-
*resultVal=pergroupstate->transValue;
1173+
*resultVal=
1174+
MakeExpandedObjectReadOnly(pergroupstate->transValue,
1175+
pergroupstate->transValueIsNull,
1176+
pertrans->transtypeLen);
11811177
*resultIsNull=pergroupstate->transValueIsNull;
11821178
}
11831179

1184-
/* If result is pass-by-ref, make sure it is in the right context. */
1185-
if (!peragg->resulttypeByVal&& !*resultIsNull&&
1186-
!MemoryContextContains(CurrentMemoryContext,
1187-
DatumGetPointer(*resultVal)))
1188-
*resultVal=datumCopy(*resultVal,
1189-
peragg->resulttypeByVal,
1190-
peragg->resulttypeLen);
1191-
11921180
MemoryContextSwitchTo(oldContext);
11931181
}
11941182

‎src/backend/executor/nodeWindowAgg.c

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -630,20 +630,13 @@ finalize_windowaggregate(WindowAggState *winstate,
630630
}
631631
else
632632
{
633-
/* Don't need MakeExpandedObjectReadOnly; datumCopy will copy it */
634-
*result=peraggstate->transValue;
633+
*result=
634+
MakeExpandedObjectReadOnly(peraggstate->transValue,
635+
peraggstate->transValueIsNull,
636+
peraggstate->transtypeLen);
635637
*isnull=peraggstate->transValueIsNull;
636638
}
637639

638-
/*
639-
* If result is pass-by-ref, make sure it is in the right context.
640-
*/
641-
if (!peraggstate->resulttypeByVal&& !*isnull&&
642-
!MemoryContextContains(CurrentMemoryContext,
643-
DatumGetPointer(*result)))
644-
*result=datumCopy(*result,
645-
peraggstate->resulttypeByVal,
646-
peraggstate->resulttypeLen);
647640
MemoryContextSwitchTo(oldContext);
648641
}
649642

@@ -1057,13 +1050,14 @@ eval_windowfunction(WindowAggState *winstate, WindowStatePerFunc perfuncstate,
10571050
*isnull=fcinfo->isnull;
10581051

10591052
/*
1060-
* Make sure pass-by-ref data is allocated in the appropriate context. (We
1061-
* need this in case the function returns a pointer into some short-lived
1062-
* tuple, as is entirely possible.)
1053+
* The window function might have returned a pass-by-ref result that's
1054+
* just a pointer into one of the WindowObject's temporary slots. That's
1055+
* not a problem if it's the only window function using the WindowObject;
1056+
* but if there's more than one function, we'd better copy the result to
1057+
* ensure it's not clobbered by later window functions.
10631058
*/
10641059
if (!perfuncstate->resulttypeByVal&& !fcinfo->isnull&&
1065-
!MemoryContextContains(CurrentMemoryContext,
1066-
DatumGetPointer(*result)))
1060+
winstate->numfuncs>1)
10671061
*result=datumCopy(*result,
10681062
perfuncstate->resulttypeByVal,
10691063
perfuncstate->resulttypeLen);
@@ -3111,6 +3105,10 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
31113105
/*
31123106
* Now we should be on the tuple immediately before or after the one we
31133107
* want, so just fetch forwards or backwards as appropriate.
3108+
*
3109+
* Notice that we tell tuplestore_gettupleslot to make a physical copy of
3110+
* the fetched tuple. This ensures that the slot's contents remain valid
3111+
* through manipulations of the tuplestore, which some callers depend on.
31143112
*/
31153113
if (winobj->seekpos>pos)
31163114
{

‎src/test/regress/expected/window.out

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -657,6 +657,23 @@ select first_value(max(x)) over (), y
657657
-> Seq Scan on tenk1
658658
(4 rows)
659659

660+
-- window functions returning pass-by-ref values from different rows
661+
select x, lag(x, 1) over (order by x), lead(x, 3) over (order by x)
662+
from (select x::numeric as x from generate_series(1,10) x);
663+
x | lag | lead
664+
----+-----+------
665+
1 | | 4
666+
2 | 1 | 5
667+
3 | 2 | 6
668+
4 | 3 | 7
669+
5 | 4 | 8
670+
6 | 5 | 9
671+
7 | 6 | 10
672+
8 | 7 |
673+
9 | 8 |
674+
10 | 9 |
675+
(10 rows)
676+
660677
-- test non-default frame specifications
661678
SELECT four, ten,
662679
sum(ten) over (partition by four order by ten),

‎src/test/regress/sql/window.sql

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,10 @@ select first_value(max(x)) over (), y
153153
from (select unique1as x, ten+fouras yfrom tenk1) ss
154154
group by y;
155155

156+
-- window functions returning pass-by-ref values from different rows
157+
select x, lag(x,1) over (order by x), lead(x,3) over (order by x)
158+
from (select x::numericas xfrom generate_series(1,10) x);
159+
156160
-- test non-default frame specifications
157161
SELECT four, ten,
158162
sum(ten) over (partition by fourorder by ten),

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp