diff options
| author | Tom Lane <tgl@sss.pgh.pa.us> | 2023-04-24 13:01:33 -0400 |
|---|---|---|
| committer | Tom Lane <tgl@sss.pgh.pa.us> | 2023-04-24 13:01:33 -0400 |
| commit | fce3b26e97ca98de054734e2af7d9125661a9b3f (patch) | |
| tree | 39507a1cc4b6aac89f6143b586fc5a2d032fd3fe /src/backend/executor/nodeAgg.c | |
| parent | 5a8366ad75eb70cffa791119b404c897983260c6 (diff) | |
| download | postgresql-fce3b26e97ca98de054734e2af7d9125661a9b3f.tar.gz postgresql-fce3b26e97ca98de054734e2af7d9125661a9b3f.zip | |
Rename ExecAggTransReparent, and improve its documentation.
The name of this function suggests that it ought to reparent R/W
expanded objects to be children of the persistent aggcontext, instead
of copying them. In fact it does no such thing, and if you try to
make it do so you will see multiple regression failures. Rename it
to the less-misleading ExecAggCopyTransValue, and add commentary
about why that attractive-sounding optimization won't work. Also
adjust comments at call sites, some of which were describing logic
that has since been moved into ExecAggCopyTransValue.
Discussion: https://postgr.es/m/3004282.1681930251@sss.pgh.pa.us
Diffstat (limited to 'src/backend/executor/nodeAgg.c')
| -rw-r--r-- | src/backend/executor/nodeAgg.c | 14 |
1 files changed, 6 insertions, 8 deletions
diff --git a/src/backend/executor/nodeAgg.c b/src/backend/executor/nodeAgg.c index 3aab5a0e80b..cfadef09420 100644 --- a/src/backend/executor/nodeAgg.c +++ b/src/backend/executor/nodeAgg.c @@ -778,12 +778,10 @@ advance_transition_function(AggState *aggstate, /* * If pass-by-ref datatype, must copy the new value into aggcontext and * free the prior transValue. But if transfn returned a pointer to its - * first input, we don't need to do anything. Also, if transfn returned a - * pointer to a R/W expanded object that is already a child of the - * aggcontext, assume we can adopt that value without copying it. + * first input, we don't need to do anything. * * It's safe to compare newVal with pergroup->transValue without regard - * for either being NULL, because ExecAggTransReparent() takes care to set + * for either being NULL, because ExecAggCopyTransValue takes care to set * transValue to 0 when NULL. Otherwise we could end up accidentally not * reparenting, when the transValue has the same numerical value as * newValue, despite being NULL. This is a somewhat hot path, making it @@ -793,10 +791,10 @@ advance_transition_function(AggState *aggstate, */ if (!pertrans->transtypeByVal && DatumGetPointer(newVal) != DatumGetPointer(pergroupstate->transValue)) - newVal = ExecAggTransReparent(aggstate, pertrans, - newVal, fcinfo->isnull, - pergroupstate->transValue, - pergroupstate->transValueIsNull); + newVal = ExecAggCopyTransValue(aggstate, pertrans, + newVal, fcinfo->isnull, + pergroupstate->transValue, + pergroupstate->transValueIsNull); pergroupstate->transValue = newVal; pergroupstate->transValueIsNull = fcinfo->isnull; |