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
Appearance settings

Commit affdde2

Browse filesBrowse files
anarazelanarazel
committed
Fix edge case leading to agg transitions skipping ExecAggTransReparent() calls.
The code checking whether an aggregate transition value needs to be reparented into the current context has always only compared the transition return value with the previous transition value by datum, i.e. without regard for NULLness. This normally works, because when the transition function returns NULL (via fcinfo->isnull), it'll return 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 turns out, it's possible to hit this case (see discussion or reproducers), leading to a non-null transition value not being reparented, followed by a crash caused by that. Instead of adding another comparison of NULLness, instead have ExecAggTransReparent() ensure that pergroup->transValue ends up as 0 when the new transition value is NULL. That avoids having to add an additional branch to the much more common cases of the transition function returning the old transition value (which is a pointer in this case), and when the new value is different, but not NULL. In branches since 69c3936, also deduplicate the reparenting code between the expression evaluation based transitions, and the path for ordered aggregates. Reported-By: Teodor Sigaev, Nikita Glukhov Author: Andres Freund Discussion: https://postgr.es/m/bd34e930-cfec-ea9b-3827-a8bc50891393@sigaev.ru Backpatch: 9.4-, this issue has existed since at least 7.4
1 parent 62c9b52 commit affdde2
Copy full SHA for affdde2

File tree

Expand file treeCollapse file tree

2 files changed

+35
-24
lines changed
Filter options
Expand file treeCollapse file tree

2 files changed

+35
-24
lines changed

‎src/backend/executor/execExprInterp.c

Copy file name to clipboardExpand all lines: src/backend/executor/execExprInterp.c
+21Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1742,6 +1742,15 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
17421742
* anything. Also, if transfn returned a pointer to a R/W
17431743
* expanded object that is already a child of the aggcontext,
17441744
* assume we can adopt that value without copying it.
1745+
*
1746+
* It's safe to compare newVal with pergroup->transValue without
1747+
* regard for either being NULL, because ExecAggTransReparent()
1748+
* takes care to set transValue to 0 when NULL. Otherwise we could
1749+
* end up accidentally not reparenting, when the transValue has
1750+
* the same numerical value as newValue, despite being NULL. This
1751+
* is a somewhat hot path, making it undesirable to instead solve
1752+
* this with another branch for the common case of the transition
1753+
* function returning its (modified) input argument.
17451754
*/
17461755
if (DatumGetPointer(newVal) != DatumGetPointer(pergroup->transValue))
17471756
newVal = ExecAggTransReparent(aggstate, pertrans,
@@ -4186,6 +4195,8 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
41864195
Datum newValue, bool newValueIsNull,
41874196
Datum oldValue, bool oldValueIsNull)
41884197
{
4198+
Assert(newValue != oldValue);
4199+
41894200
if (!newValueIsNull)
41904201
{
41914202
MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory);
@@ -4199,6 +4210,16 @@ ExecAggTransReparent(AggState *aggstate, AggStatePerTrans pertrans,
41994210
pertrans->transtypeByVal,
42004211
pertrans->transtypeLen);
42014212
}
4213+
else
4214+
{
4215+
/*
4216+
* Ensure that AggStatePerGroup->transValue ends up being 0, so
4217+
* callers can safely compare newValue/oldValue without having to
4218+
* check their respective nullness.
4219+
*/
4220+
newValue = (Datum) 0;
4221+
}
4222+
42024223
if (!oldValueIsNull)
42034224
{
42044225
if (DatumIsReadWriteExpandedObject(oldValue,

‎src/backend/executor/nodeAgg.c

Copy file name to clipboardExpand all lines: src/backend/executor/nodeAgg.c
+14-24Lines 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"
@@ -626,33 +627,22 @@ advance_transition_function(AggState *aggstate,
626627
* first input, we don't need to do anything. Also, if transfn returned a
627628
* pointer to a R/W expanded object that is already a child of the
628629
* aggcontext, assume we can adopt that value without copying it.
630+
*
631+
* It's safe to compare newVal with pergroup->transValue without
632+
* regard for either being NULL, because ExecAggTransReparent()
633+
* takes care to set transValue to 0 when NULL. Otherwise we could
634+
* end up accidentally not reparenting, when the transValue has
635+
* the same numerical value as newValue, despite being NULL. This
636+
* is a somewhat hot path, making it undesirable to instead solve
637+
* this with another branch for the common case of the transition
638+
* function returning its (modified) input argument.
629639
*/
630640
if (!pertrans->transtypeByVal &&
631641
DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue))
632-
{
633-
if (!fcinfo->isnull)
634-
{
635-
MemoryContextSwitchTo(aggstate->curaggcontext->ecxt_per_tuple_memory);
636-
if (DatumIsReadWriteExpandedObject(newVal,
637-
false,
638-
pertrans->transtypeLen) &&
639-
MemoryContextGetParent(DatumGetEOHP(newVal)->eoh_context) == CurrentMemoryContext)
640-
/* do nothing */ ;
641-
else
642-
newVal = datumCopy(newVal,
643-
pertrans->transtypeByVal,
644-
pertrans->transtypeLen);
645-
}
646-
if (!pergroupstate->transValueIsNull)
647-
{
648-
if (DatumIsReadWriteExpandedObject(pergroupstate->transValue,
649-
false,
650-
pertrans->transtypeLen))
651-
DeleteExpandedObject(pergroupstate->transValue);
652-
else
653-
pfree(DatumGetPointer(pergroupstate->transValue));
654-
}
655-
}
642+
newVal = ExecAggTransReparent(aggstate, pertrans,
643+
newVal, fcinfo->isnull,
644+
pergroupstate->transValue,
645+
pergroupstate->transValueIsNull);
656646

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

0 commit comments

Comments
0 (0)
Morty Proxy This is a proxified and sanitized view of the page, visit original site.