Thread: Free list same_input_transnos in preprocess_aggref
Hi,
In preprocess_aggref(), list same_input_transnos is used to track compatible transnos.
Free it if we don’t need it anymore.
```
/*
* 2. See if this aggregate can share transition state with another
* aggregate that we've initialized already.
*/
transno = find_compatible_trans(root, aggref, shareable,
aggtransfn, aggtranstype,
transtypeLen, transtypeByVal,
aggcombinefn,
aggserialfn, aggdeserialfn,
initValue, initValueIsNull,
same_input_transnos);
list_free(same_input_transnos);
```
Not sure if it worths as it will be freed sooner or later when current context ends.
But as in find_compatible_agg(), the list is freed if we found a compatible Agg.
This patch helps a little when there are lots of incompatible aggs because we will try to find the compatible transnos again and again.
Each iteration will keep an unused list memory.
In preprocess_aggref(), list same_input_transnos is used to track compatible transnos.
Free it if we don’t need it anymore.
```
/*
* 2. See if this aggregate can share transition state with another
* aggregate that we've initialized already.
*/
transno = find_compatible_trans(root, aggref, shareable,
aggtransfn, aggtranstype,
transtypeLen, transtypeByVal,
aggcombinefn,
aggserialfn, aggdeserialfn,
initValue, initValueIsNull,
same_input_transnos);
list_free(same_input_transnos);
```
Not sure if it worths as it will be freed sooner or later when current context ends.
But as in find_compatible_agg(), the list is freed if we found a compatible Agg.
This patch helps a little when there are lots of incompatible aggs because we will try to find the compatible transnos again and again.
Each iteration will keep an unused list memory.
Regards,
Zhang Mingli
Attachment
Zhang Mingli <zmlpostgres@gmail.com> writes: > In preprocess_aggref(), list same_input_transnos is used to track compatible transnos. > Free it if we don’t need it anymore. Very little of the planner bothers with freeing small allocations like that. Can you demonstrate a case where this would actually make a meaningful difference? regards, tom lane
Regards,
Zhang Mingli
On Sep 19, 2022, 23:14 +0800, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
Very little of the planner bothers with freeing small allocations
like that.
I think so too, as said, not sure if it worths.
Can you demonstrate a case where this would actually
make a meaningful difference?
Offhand, an example may help a little:
create table t1(id int);
explain select max(id), min(id), sum(id), count(id), avg(id) from t1;
Modify codes to test:
@@ -139,6 +139,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
int16 transtypeLen;
Oid inputTypes[FUNC_MAX_ARGS];
int numArguments;
+ static size_t accumulate_list_size = 0;
Assert(aggref->agglevelsup == 0);
@@ -265,7 +266,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
aggserialfn, aggdeserialfn,
initValue, initValueIsNull,
same_input_transnos);
- list_free(same_input_transnos);
+ accumulate_list_size += sizeof(int) * list_length(same_input_transnos);
Gdb and print accumulate_list_size for each iteration:
SaveBytes = Sum results of accumulate_list_size: 32(4+4+8+8), as we have 5 aggs in sql.
If there were N sets of that aggs (more columns as id, with above aggs ), the bytes will be N*SaveBytes.
Seems we don’t have so many agg functions that could share the same trans function, Does it worth?
create table t1(id int);
explain select max(id), min(id), sum(id), count(id), avg(id) from t1;
Modify codes to test:
@@ -139,6 +139,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
int16 transtypeLen;
Oid inputTypes[FUNC_MAX_ARGS];
int numArguments;
+ static size_t accumulate_list_size = 0;
Assert(aggref->agglevelsup == 0);
@@ -265,7 +266,7 @@ preprocess_aggref(Aggref *aggref, PlannerInfo *root)
aggserialfn, aggdeserialfn,
initValue, initValueIsNull,
same_input_transnos);
- list_free(same_input_transnos);
+ accumulate_list_size += sizeof(int) * list_length(same_input_transnos);
Gdb and print accumulate_list_size for each iteration:
SaveBytes = Sum results of accumulate_list_size: 32(4+4+8+8), as we have 5 aggs in sql.
If there were N sets of that aggs (more columns as id, with above aggs ), the bytes will be N*SaveBytes.
Seems we don’t have so many agg functions that could share the same trans function, Does it worth?
Regards,
Zhang Mingli
On Sep 20, 2022, 00:27 +0800, Zhang Mingli <zmlpostgres@gmail.com>, wrote:
SaveBytes = Sum results of accumulate_list_size: 32(4+4+8+8), as we have 5 aggs in sql
Correction: SaveBytes = Sum results of accumulate_list_size: 24(4+4+8+8),
Zhang Mingli <zmlpostgres@gmail.com> writes: > Correction: SaveBytes = Sum results of accumulate_list_size: 24(4+4+8+8), What I did was to stick in elog(LOG, "leaking list of length %d", list_length(same_input_transnos)); at the end of preprocess_aggref. What I see on your five-aggregate example is 2022-11-06 14:59:25.666 EST [3046253] LOG: leaking list of length 0 2022-11-06 14:59:25.666 EST [3046253] STATEMENT: explain select max(id), min(id), sum(id), count(id), avg(id) from t1; 2022-11-06 14:59:25.666 EST [3046253] LOG: leaking list of length 1 2022-11-06 14:59:25.666 EST [3046253] STATEMENT: explain select max(id), min(id), sum(id), count(id), avg(id) from t1; 2022-11-06 14:59:25.666 EST [3046253] LOG: leaking list of length 0 2022-11-06 14:59:25.666 EST [3046253] STATEMENT: explain select max(id), min(id), sum(id), count(id), avg(id) from t1; 2022-11-06 14:59:25.666 EST [3046253] LOG: leaking list of length 1 2022-11-06 14:59:25.666 EST [3046253] STATEMENT: explain select max(id), min(id), sum(id), count(id), avg(id) from t1; 2022-11-06 14:59:25.666 EST [3046253] LOG: leaking list of length 0 2022-11-06 14:59:25.666 EST [3046253] STATEMENT: explain select max(id), min(id), sum(id), count(id), avg(id) from t1; The NIL lists are of course occupying no storage. The two one-element lists are absolutely, completely negligible in the context of planning any nontrivial statement. Even the aggtransinfos list that is the primary output of preprocess_aggref will dwarf that; and we leak similarly small data structures in probably many hundred places in the planner. I went a bit further and ran the core regression tests, then aggregated the results: $ grep 'leaking list' postmaster.log | sed 's/.*] //' | sort | uniq -c 4516 LOG: leaking list of length 0 95 LOG: leaking list of length 1 15 LOG: leaking list of length 2 You can quibble of course about how representative the regression tests are, but there's sure no evidence at all here that we'd be saving anything measurable. If anything, I'd be inclined to get rid of the list_free(*same_input_transnos); in find_compatible_agg, because it seems like a waste of code on the same grounds. Instrumenting that in the same way, I find that it's not reached at all in your example, while the regression tests give 49 LOG: freeing list of length 0 2 LOG: freeing list of length 1 regards, tom lane
HI,
On Nov 7, 2022, 04:12 +0800, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
On Nov 7, 2022, 04:12 +0800, Tom Lane <tgl@sss.pgh.pa.us>, wrote:
The NIL lists are of course occupying no storage. The two one-element
lists are absolutely, completely negligible in the context of planning
any nontrivial statement. Even the aggtransinfos list that is the
primary output of preprocess_aggref will dwarf that; and we leak
similarly small data structures in probably many hundred places in
the planner.
I went a bit further and ran the core regression tests, then aggregated
the results:
$ grep 'leaking list' postmaster.log | sed 's/.*] //' | sort | uniq -c
4516 LOG: leaking list of length 0
95 LOG: leaking list of length 1
15 LOG: leaking list of length 2
You can quibble of course about how representative the regression tests
are, but there's sure no evidence at all here that we'd be saving
anything measurable.
If anything, I'd be inclined to get rid of the
list_free(*same_input_transnos);
in find_compatible_agg, because it seems like a waste of code on
the same grounds. Instrumenting that in the same way, I find
that it's not reached at all in your example, while the
regression tests give
49 LOG: freeing list of length 0
2 LOG: freeing list of length 1
Thanks for the investigation.
Yeah, this patch is negligible. I’ll withdraw it in CF.
Yeah, this patch is negligible. I’ll withdraw it in CF.
Regards,
Zhang Mingli