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

Commitfce3b26

Browse files
committed
Rename ExecAggTransReparent, and improve its documentation.
The name of this function suggests that it ought to reparent R/Wexpanded objects to be children of the persistent aggcontext, insteadof copying them. In fact it does no such thing, and if you try tomake it do so you will see multiple regression failures. Rename itto the less-misleading ExecAggCopyTransValue, and add commentaryabout why that attractive-sounding optimization won't work. Alsoadjust comments at call sites, some of which were describing logicthat has since been moved into ExecAggCopyTransValue.Discussion:https://postgr.es/m/3004282.1681930251@sss.pgh.pa.us
1 parent5a8366a commitfce3b26

File tree

6 files changed

+51
-28
lines changed

6 files changed

+51
-28
lines changed

‎src/backend/executor/execExprInterp.c

Lines changed: 36 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -4341,15 +4341,40 @@ ExecAggInitGroup(AggState *aggstate, AggStatePerTrans pertrans, AggStatePerGroup
43414341
}
43424342

43434343
/*
4344-
* Ensure that the current transition value is a child of the aggcontext,
4345-
* rather than the per-tuple context.
4344+
* Ensure that the new transition value is stored in the aggcontext,
4345+
* rather than the per-tuple context. This should be invoked only when
4346+
* we know (a) the transition data type is pass-by-reference, and (b)
4347+
* the newValue is distinct from the oldValue.
43464348
*
43474349
* NB: This can change the current memory context.
4350+
*
4351+
* We copy the presented newValue into the aggcontext, except when the datum
4352+
* points to a R/W expanded object that is already a child of the aggcontext,
4353+
* in which case we need not copy. We then delete the oldValue, if not null.
4354+
*
4355+
* If the presented datum points to a R/W expanded object that is a child of
4356+
* some other context, ideally we would just reparent it under the aggcontext.
4357+
* Unfortunately, that doesn't work easily, and it wouldn't help anyway for
4358+
* aggregate-aware transfns. We expect that a transfn that deals in expanded
4359+
* objects and is aware of the memory management conventions for aggregate
4360+
* transition values will (1) on first call, return a R/W expanded object that
4361+
* is already in the right context, allowing us to do nothing here, and (2) on
4362+
* subsequent calls, modify and return that same object, so that control
4363+
* doesn't even reach here. However, if we have a generic transfn that
4364+
* returns a new R/W expanded object (probably in the per-tuple context),
4365+
* reparenting that result would cause problems. We'd pass that R/W object to
4366+
* the next invocation of the transfn, and then it would be at liberty to
4367+
* change or delete that object, and if it deletes it then our own attempt to
4368+
* delete the now-old transvalue afterwards would be a double free. We avoid
4369+
* this problem by forcing the stored transvalue to always be a flat
4370+
* non-expanded object unless the transfn is visibly doing aggregate-aware
4371+
* memory management. This is somewhat inefficient, but the best answer to
4372+
* that is to write a smarter transfn.
43484373
*/
43494374
Datum
4350-
ExecAggTransReparent(AggState*aggstate,AggStatePerTranspertrans,
4351-
DatumnewValue,boolnewValueIsNull,
4352-
DatumoldValue,boololdValueIsNull)
4375+
ExecAggCopyTransValue(AggState*aggstate,AggStatePerTranspertrans,
4376+
DatumnewValue,boolnewValueIsNull,
4377+
DatumoldValue,boololdValueIsNull)
43534378
{
43544379
Assert(newValue!=oldValue);
43554380

@@ -4559,12 +4584,10 @@ ExecAggPlainTransByRef(AggState *aggstate, AggStatePerTrans pertrans,
45594584
/*
45604585
* For pass-by-ref datatype, must copy the new value into aggcontext and
45614586
* free the prior transValue. But if transfn returned a pointer to its
4562-
* first input, we don't need to do anything. Also, if transfn returned a
4563-
* pointer to a R/W expanded object that is already a child of the
4564-
* aggcontext, assume we can adopt that value without copying it.
4587+
* first input, we don't need to do anything.
45654588
*
45664589
* It's safe to compare newVal with pergroup->transValue without regard
4567-
* for either being NULL, becauseExecAggTransReparent() takes care to set
4590+
* for either being NULL, becauseExecAggCopyTransValue takes care to set
45684591
* transValue to 0 when NULL. Otherwise we could end up accidentally not
45694592
* reparenting, when the transValue has the same numerical value as
45704593
* newValue, despite being NULL. This is a somewhat hot path, making it
@@ -4573,10 +4596,10 @@ ExecAggPlainTransByRef(AggState *aggstate, AggStatePerTrans pertrans,
45734596
* argument.
45744597
*/
45754598
if (DatumGetPointer(newVal)!=DatumGetPointer(pergroup->transValue))
4576-
newVal=ExecAggTransReparent(aggstate,pertrans,
4577-
newVal,fcinfo->isnull,
4578-
pergroup->transValue,
4579-
pergroup->transValueIsNull);
4599+
newVal=ExecAggCopyTransValue(aggstate,pertrans,
4600+
newVal,fcinfo->isnull,
4601+
pergroup->transValue,
4602+
pergroup->transValueIsNull);
45804603

45814604
pergroup->transValue=newVal;
45824605
pergroup->transValueIsNull=fcinfo->isnull;

‎src/backend/executor/nodeAgg.c

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -778,12 +778,10 @@ advance_transition_function(AggState *aggstate,
778778
/*
779779
* If pass-by-ref datatype, must copy the new value into aggcontext and
780780
* free the prior transValue. But if transfn returned a pointer to its
781-
* first input, we don't need to do anything. Also, if transfn returned a
782-
* pointer to a R/W expanded object that is already a child of the
783-
* aggcontext, assume we can adopt that value without copying it.
781+
* first input, we don't need to do anything.
784782
*
785783
* It's safe to compare newVal with pergroup->transValue without regard
786-
* for either being NULL, becauseExecAggTransReparent() takes care to set
784+
* for either being NULL, becauseExecAggCopyTransValue takes care to set
787785
* transValue to 0 when NULL. Otherwise we could end up accidentally not
788786
* reparenting, when the transValue has the same numerical value as
789787
* newValue, despite being NULL. This is a somewhat hot path, making it
@@ -793,10 +791,10 @@ advance_transition_function(AggState *aggstate,
793791
*/
794792
if (!pertrans->transtypeByVal&&
795793
DatumGetPointer(newVal)!=DatumGetPointer(pergroupstate->transValue))
796-
newVal=ExecAggTransReparent(aggstate,pertrans,
797-
newVal,fcinfo->isnull,
798-
pergroupstate->transValue,
799-
pergroupstate->transValueIsNull);
794+
newVal=ExecAggCopyTransValue(aggstate,pertrans,
795+
newVal,fcinfo->isnull,
796+
pergroupstate->transValue,
797+
pergroupstate->transValueIsNull);
800798

801799
pergroupstate->transValue=newVal;
802800
pergroupstate->transValueIsNull=fcinfo->isnull;

‎src/backend/executor/nodeWindowAgg.c

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -368,7 +368,8 @@ advance_windowaggregate(WindowAggState *winstate,
368368
* free the prior transValue. But if transfn returned a pointer to its
369369
* first input, we don't need to do anything. Also, if transfn returned a
370370
* pointer to a R/W expanded object that is already a child of the
371-
* aggcontext, assume we can adopt that value without copying it.
371+
* aggcontext, assume we can adopt that value without copying it. (See
372+
* comments for ExecAggCopyTransValue, which this code duplicates.)
372373
*/
373374
if (!peraggstate->transtypeByVal&&
374375
DatumGetPointer(newVal)!=DatumGetPointer(peraggstate->transValue))
@@ -533,7 +534,8 @@ advance_windowaggregate_base(WindowAggState *winstate,
533534
* free the prior transValue. But if invtransfn returned a pointer to its
534535
* first input, we don't need to do anything. Also, if invtransfn
535536
* returned a pointer to a R/W expanded object that is already a child of
536-
* the aggcontext, assume we can adopt that value without copying it.
537+
* the aggcontext, assume we can adopt that value without copying it. (See
538+
* comments for ExecAggCopyTransValue, which this code duplicates.)
537539
*
538540
* Note: the checks for null values here will never fire, but it seems
539541
* best to have this stanza look just like advance_windowaggregate.

‎src/backend/jit/llvm/llvmjit_expr.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2314,7 +2314,7 @@ llvm_compile_expr(ExprState *state)
23142314
params[5]=LLVMBuildTrunc(b,v_transnull,
23152315
TypeParamBool,"");
23162316

2317-
v_fn=llvm_pg_func(mod,"ExecAggTransReparent");
2317+
v_fn=llvm_pg_func(mod,"ExecAggCopyTransValue");
23182318
v_newval=
23192319
LLVMBuildCall(b,v_fn,
23202320
params,lengthof(params),

‎src/backend/jit/llvm/llvmjit_types.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -102,7 +102,7 @@ FunctionReturningBool(void)
102102
void*referenced_functions[]=
103103
{
104104
ExecAggInitGroup,
105-
ExecAggTransReparent,
105+
ExecAggCopyTransValue,
106106
ExecEvalPreOrderedDistinctSingle,
107107
ExecEvalPreOrderedDistinctMulti,
108108
ExecEvalAggOrderedTransDatum,

‎src/include/executor/execExpr.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -807,9 +807,9 @@ extern void ExecEvalSysVar(ExprState *state, ExprEvalStep *op,
807807

808808
externvoidExecAggInitGroup(AggState*aggstate,AggStatePerTranspertrans,AggStatePerGrouppergroup,
809809
ExprContext*aggcontext);
810-
externDatumExecAggTransReparent(AggState*aggstate,AggStatePerTranspertrans,
811-
DatumnewValue,boolnewValueIsNull,
812-
DatumoldValue,boololdValueIsNull);
810+
externDatumExecAggCopyTransValue(AggState*aggstate,AggStatePerTranspertrans,
811+
DatumnewValue,boolnewValueIsNull,
812+
DatumoldValue,boololdValueIsNull);
813813
externboolExecEvalPreOrderedDistinctSingle(AggState*aggstate,
814814
AggStatePerTranspertrans);
815815
externboolExecEvalPreOrderedDistinctMulti(AggState*aggstate,

0 commit comments

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