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

Commitc8e0e56

Browse files
committed
Fix edge case leading to agg transitions skipping ExecAggTransReparent() calls.
The code checking whether an aggregate transition value needs to bereparented into the current context has always only compared thetransition return value with the previous transition value by datum,i.e. without regard for NULLness. This normally works, because whenthe transition function returns NULL (via fcinfo->isnull), it'llreturn a value that won't be the same as its input value.But there's no hard requirement that that's the case. And it turnsout, it's possible to hit this case (see discussion or reproducers),leading to a non-null transition value not being reparented, followedby a crash caused by that.Instead of adding another comparison of NULLness, instead haveExecAggTransReparent() ensure that pergroup->transValue ends up as 0when the new transition value is NULL. That avoids having to add anadditional branch to the much more common cases of the transitionfunction returning the old transition value (which is a pointer inthis case), and when the new value is different, but not NULL.In branches since69c3936, also deduplicate the reparenting codebetween the expression evaluation based transitions, and the path forordered aggregates.Reported-By: Teodor Sigaev, Nikita GlukhovAuthor: Andres FreundDiscussion:https://postgr.es/m/bd34e930-cfec-ea9b-3827-a8bc50891393@sigaev.ruBackpatch: 9.4-, this issue has existed since at least 7.4
1 parent7c7026b commitc8e0e56

File tree

2 files changed

+35
-24
lines changed

2 files changed

+35
-24
lines changed

‎src/backend/executor/execExprInterp.c‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1719,6 +1719,15 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
17191719
* anything. Also, if transfn returned a pointer to a R/W
17201720
* expanded object that is already a child of the aggcontext,
17211721
* assume we can adopt that value without copying it.
1722+
*
1723+
* It's safe to compare newVal with pergroup->transValue without
1724+
* regard for either being NULL, because ExecAggTransReparent()
1725+
* takes care to set transValue to 0 when NULL. Otherwise we could
1726+
* end up accidentally not reparenting, when the transValue has
1727+
* the same numerical value as newValue, despite being NULL. This
1728+
* is a somewhat hot path, making it undesirable to instead solve
1729+
* this with another branch for the common case of the transition
1730+
* function returning its (modified) input argument.
17221731
*/
17231732
if (DatumGetPointer(newVal)!=DatumGetPointer(pergroup->transValue))
17241733
newVal=ExecAggTransReparent(aggstate,pertrans,
@@ -4043,6 +4052,8 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
40434052
DatumnewValue,boolnewValueIsNull,
40444053
DatumoldValue,boololdValueIsNull)
40454054
{
4055+
Assert(newValue!=oldValue);
4056+
40464057
if (!newValueIsNull)
40474058
{
40484059
MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory);
@@ -4056,6 +4067,16 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
40564067
pertrans->transtypeByVal,
40574068
pertrans->transtypeLen);
40584069
}
4070+
else
4071+
{
4072+
/*
4073+
* Ensure that AggStatePerGroup->transValue ends up being 0, so
4074+
* callers can safely compare newValue/oldValue without having to
4075+
* check their respective nullness.
4076+
*/
4077+
newValue= (Datum)0;
4078+
}
4079+
40594080
if (!oldValueIsNull)
40604081
{
40614082
if (DatumIsReadWriteExpandedObject(oldValue,

‎src/backend/executor/nodeAgg.c‎

Lines changed: 14 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,7 @@
221221
#include"catalog/pg_aggregate.h"
222222
#include"catalog/pg_proc.h"
223223
#include"catalog/pg_type.h"
224+
#include"executor/execExpr.h"
224225
#include"executor/executor.h"
225226
#include"executor/nodeAgg.h"
226227
#include"miscadmin.h"
@@ -627,33 +628,22 @@ advance_transition_function(AggState *aggstate,
627628
* first input, we don't need to do anything. Also, if transfn returned a
628629
* pointer to a R/W expanded object that is already a child of the
629630
* aggcontext, assume we can adopt that value without copying it.
631+
*
632+
* It's safe to compare newVal with pergroup->transValue without
633+
* regard for either being NULL, because ExecAggTransReparent()
634+
* takes care to set transValue to 0 when NULL. Otherwise we could
635+
* end up accidentally not reparenting, when the transValue has
636+
* the same numerical value as newValue, despite being NULL. This
637+
* is a somewhat hot path, making it undesirable to instead solve
638+
* this with another branch for the common case of the transition
639+
* function returning its (modified) input argument.
630640
*/
631641
if (!pertrans->transtypeByVal&&
632642
DatumGetPointer(newVal)!=DatumGetPointer(pergroupstate->transValue))
633-
{
634-
if (!fcinfo->isnull)
635-
{
636-
MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory);
637-
if (DatumIsReadWriteExpandedObject(newVal,
638-
false,
639-
pertrans->transtypeLen)&&
640-
MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context)==CurrentMemoryContext)
641-
/* do nothing */ ;
642-
else
643-
newVal=datumCopy(newVal,
644-
pertrans->transtypeByVal,
645-
pertrans->transtypeLen);
646-
}
647-
if (!pergroupstate->transValueIsNull)
648-
{
649-
if (DatumIsReadWriteExpandedObject(pergroupstate->transValue,
650-
false,
651-
pertrans->transtypeLen))
652-
DeleteExpandedObject(pergroupstate->transValue);
653-
else
654-
pfree(DatumGetPointer(pergroupstate->transValue));
655-
}
656-
}
643+
newVal=ExecAggTransReparent(aggstate,pertrans,
644+
newVal,fcinfo->isnull,
645+
pergroupstate->transValue,
646+
pergroupstate->transValueIsNull);
657647

658648
pergroupstate->transValue=newVal;
659649
pergroupstate->transValueIsNull=fcinfo->isnull;

0 commit comments

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