Skip to content

Commit

Permalink
rowexec: fix usage of the shared "single datum" acc in windower
Browse files Browse the repository at this point in the history
This commit fixes the usage of the shared "single datum agg mem account"
by the window builtins. Previously, we were updating the eval context
with the memory account after the builtins were constructed. The impact
of the bug on its own is pretty minor (namely that we could reserve more
memory than necessary - the initial allocation is 10KiB), but I have
some other changes brewing which make it more important to be precise
about the attribution of the memory allocations.

Release note: None
  • Loading branch information
yuzefovich committed Sep 30, 2022
1 parent cdcd49e commit 982eb16
Showing 1 changed file with 20 additions and 19 deletions.
39 changes: 20 additions & 19 deletions pkg/sql/rowexec/windower.go
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,26 @@ func newWindower(
w.inputTypes = input.OutputTypes()
ctx := evalCtx.Ctx()

// Limit the memory use by creating a child monitor with a hard limit.
// windower will overflow to disk if this limit is not enough.
limit := execinfra.GetWorkMemLimit(flowCtx)
if limit < memRequiredByWindower {
// The limit is set very low (likely by the tests in order to improve
// the test coverage), but the windower requires some amount of RAM, so
// we override the limit. This behavior is acceptable given that we
// don't expect anyone to lower the setting to less than 100KiB in
// production.
limit = memRequiredByWindower
}
limitedMon := mon.NewMonitorInheritWithLimit("windower-limited", limit, evalCtx.Mon)
limitedMon.StartNoReserved(ctx, evalCtx.Mon)
w.acc = limitedMon.MakeBoundAccount()
// If we have aggregate builtins that aggregate a single datum, we want
// them to reuse the same shared memory account with the windower. Notably,
// we need to update the eval context before constructing the window
// builtins.
evalCtx.SingleDatumAggMemAccount = &w.acc

w.partitionBy = spec.PartitionBy
windowFns := spec.WindowFns
w.windowFns = make([]*windowFunc, 0, len(windowFns))
Expand Down Expand Up @@ -145,20 +165,6 @@ func newWindower(
}
w.outputRow = make(rowenc.EncDatumRow, len(w.outputTypes))

// Limit the memory use by creating a child monitor with a hard limit.
// windower will overflow to disk if this limit is not enough.
limit := execinfra.GetWorkMemLimit(flowCtx)
if limit < memRequiredByWindower {
// The limit is set very low (likely by the tests in order to improve
// the test coverage), but the windower requires some amount of RAM, so
// we override the limit. This behavior is acceptable given that we
// don't expect anyone to lower the setting to less than 100KiB in
// production.
limit = memRequiredByWindower
}
limitedMon := mon.NewMonitorInheritWithLimit("windower-limited", limit, evalCtx.Mon)
limitedMon.StartNoReserved(ctx, evalCtx.Mon)

if err := w.InitWithEvalCtx(
w,
post,
Expand Down Expand Up @@ -191,11 +197,6 @@ func newWindower(
return nil, err
}

w.acc = w.MemMonitor.MakeBoundAccount()
// If we have aggregate builtins that aggregate a single datum, we want
// them to reuse the same shared memory account with the windower.
evalCtx.SingleDatumAggMemAccount = &w.acc

if execstats.ShouldCollectStats(ctx, flowCtx.CollectStats) {
w.input = newInputStatCollector(w.input)
w.ExecStatsForTrace = w.execStatsForTrace
Expand Down

0 comments on commit 982eb16

Please sign in to comment.