sql: account for memory of results of window functions computations #38839
Conversation
|
This change is |
|
Local benchmarks don't show any significant difference (they actually show a slight performance improvement that I would explain by noise): |
There was a problem hiding this comment.
Can you add a test with a small memory account that proves that queries of this form will hit the limit when we expec them to?
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @asubiotto and @jordanlewis)
yuzefovich
force-pushed
the
wf-fix-memory
branch
from
781306d to
dcbb839
Compare
There was a problem hiding this comment.
I added a test, and while looking closer to the code, I realized that we were, in fact, accounting for the results of computations, but only as nil Datums. So I think now the memory monitoring is correct - first, when we allocate a slice for windowValues, we use the default Datum size times the number of rows; then, once we know the size of the datum, we update the account accordingly. Jordan, could you please take a quick look at this to confirm my logic?
Reviewable status:
pkg/sql/distsqlrun/windower.go, line 555 at r1 (raw file):
builtin.Reset(ctx) usage = datumSliceOverhead + sizeOfDatum*int64(partition.Len())
This is where we account for the slice with default datum size.
yuzefovich
force-pushed
the
wf-fix-memory
branch
from
dcbb839 to
b1020f7
Compare
|
I also noticed that there was a bug in benchmarks of windower - Memory account fix doesn't have any significant performance difference, but the true speed went down by about 50%: |
yuzefovich
force-pushed
the
wf-fix-memory
branch
2 times, most recently
from
0fdf3d2 to
eac0516
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/sql/distsqlrun/windower_test.go, line 64 at r2 (raw file):
} t.Run("", func(t *testing.T) {
nit: there is no need to have this. Subtests aren't a requirement - having no t.Run will mean a single top level test.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r1, 1 of 1 files at r2.
Reviewable status:
Previously, Datums that were the result of computing of window functions were accounted for in memory monitoring only as nils which could lead to OOM when the actual Datum's size was a lot bigger than default one (array_agg and concat_agg were most prone to this). Now, the underlying memory is tracked correctly. Release note: None
yuzefovich
force-pushed
the
wf-fix-memory
branch
from
eac0516 to
0d5409d
Compare
There was a problem hiding this comment.
TFTR!
bors r+
Reviewable status:
pkg/sql/distsqlrun/windower_test.go, line 64 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
nit: there is no need to have this. Subtests aren't a requirement - having no t.Run will mean a single top level test.
Thanks, fixed.
38839: sql: account for memory of results of window functions computations r=yuzefovich a=yuzefovich Previously, Datums that were the result of computing of window functions were accounted for in memory monitoring only as nils which could lead to OOM when the actual Datum's size was a lot bigger than default one (array_agg and concat_agg were most prone to this). Now, the underlying memory is tracked correctly. Fixes: #38818. Release note: None Co-authored-by: Yahor Yuzefovich <[email protected]>
Build succeeded |
Previously, Datums that were the result of computing of window
functions were accounted for in memory monitoring only as nils which
could lead to OOM when the actual Datum's size was a lot bigger than
default one (array_agg and concat_agg were most prone to this).
Now, the underlying memory is tracked correctly.
Fixes: #38818.
Release note: None