diff --git a/pkg/sql/builtin_mem_usage_test.go b/pkg/sql/builtin_mem_usage_test.go index 507c2ced372b..502e9987aece 100644 --- a/pkg/sql/builtin_mem_usage_test.go +++ b/pkg/sql/builtin_mem_usage_test.go @@ -17,14 +17,10 @@ package sql import ( "context" gosql "database/sql" - "fmt" "testing" "github.com/cockroachdb/cockroach/pkg/base" - "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" - "github.com/cockroachdb/cockroach/pkg/sql/sem/builtins" - "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/lib/pq" @@ -96,71 +92,14 @@ func TestAggregatesMonitorMemory(t *testing.T) { } } -func TestBuiltinsAccountForMemory(t *testing.T) { - defer leaktest.AfterTest(t)() - - _, repeatFns := builtins.GetBuiltinProperties("repeat") - _, concatFns := builtins.GetBuiltinProperties("concat") - _, concatwsFns := builtins.GetBuiltinProperties("concat_ws") - _, lowerFns := builtins.GetBuiltinProperties("lower") - - testData := []struct { - builtin tree.Overload - args tree.Datums - expectedAllocation int64 - }{ - {repeatFns[0], - tree.Datums{ - tree.NewDString("abc"), - tree.NewDInt(123), - }, - int64(3 * 123)}, - {concatFns[0], - tree.Datums{ - tree.NewDString("abc"), - tree.NewDString("abc"), - }, - int64(3 + 3)}, - {concatwsFns[0], - tree.Datums{ - tree.NewDString("!"), - tree.NewDString("abc"), - tree.NewDString("abc"), - }, - int64(3 + 1 + 3)}, - {lowerFns[0], - tree.Datums{ - tree.NewDString("ABC"), - }, - int64(3)}, - } - - for _, test := range testData { - t.Run("", func(t *testing.T) { - evalCtx := tree.NewTestingEvalContext(cluster.MakeTestingClusterSettings()) - defer evalCtx.Stop(context.Background()) - defer evalCtx.ActiveMemAcc.Close(context.Background()) - previouslyAllocated := evalCtx.ActiveMemAcc.Used() - _, err := test.builtin.Fn(evalCtx, test.args) - if err != nil { - t.Fatal(err) - } - deltaAllocated := evalCtx.ActiveMemAcc.Used() - previouslyAllocated - if deltaAllocated != test.expectedAllocation { - t.Errorf("Expected to allocate %d, actually allocated %d", test.expectedAllocation, deltaAllocated) - } - }) - } -} - func TestEvaluatedMemoryIsChecked(t *testing.T) { defer leaktest.AfterTest(t)() // We select the LENGTH here and elsewhere because if we passed the result of // REPEAT up as a result, the memory error would be caught there even if // REPEAT was not doing its accounting. testData := []string{ - `SELECT length(repeat('abc', 300000))`, - `SELECT crdb_internal.no_constant_folding(length(repeat('abc', 300000)))`, + `SELECT length(repeat('abc', 70000000))`, + `SELECT crdb_internal.no_constant_folding(length(repeat('abc', 70000000)))`, } for _, statement := range testData { @@ -172,45 +111,9 @@ func TestEvaluatedMemoryIsChecked(t *testing.T) { if _, err := sqlDB.Exec( statement, - ); err.(*pq.Error).Code != pgerror.CodeOutOfMemoryError { + ); err.(*pq.Error).Code != pgerror.CodeProgramLimitExceededError { t.Errorf("Expected \"%s\" to OOM, but it didn't", statement) } }) } } - -func TestMemoryGetsFreedOnEachRow(t *testing.T) { - defer leaktest.AfterTest(t)() - // This test verifies that the memory allocated during the computation of a - // row gets freed before moving on to subsequent rows. - - s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{ - SQLMemoryPoolSize: lowMemoryBudget, - }) - defer s.Stopper().Stop(context.Background()) - - stringLength := 300000 - numRows := 100 - - // Check that if this string is allocated per-row, we don't OOM. - if _, err := sqlDB.Exec( - fmt.Sprintf( - `SELECT crdb_internal.no_constant_folding(length(repeat('a', %d))) FROM generate_series(1, %d)`, - stringLength, - numRows, - ), - ); err != nil { - t.Fatalf("Expected statement to run successfully, but got %s", err) - } - - // Ensure that if this memory is all allocated at once, we OOM. - if _, err := sqlDB.Exec( - fmt.Sprintf( - `SELECT crdb_internal.no_constant_folding(length(repeat('a', %d * %d)))`, - stringLength, - numRows, - ), - ); err.(*pq.Error).Code != pgerror.CodeOutOfMemoryError { - t.Fatalf("Expected statement to OOM, but it didn't") - } -} diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index d6eb608e55bf..f3828b35a51a 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -414,12 +414,6 @@ func (ex *connExecutor) execStmtInOpenState( // contexts. p.cancelChecker = sqlbase.NewCancelChecker(ctx) - // constantMemAcc accounts for all constant folded values that are computed - // prior to any rows being computed. - constantMemAcc := p.EvalContext().Mon.MakeBoundAccount() - p.EvalContext().ActiveMemAcc = &constantMemAcc - defer constantMemAcc.Close(ctx) - if runInParallel { cols, err := ex.execStmtInParallel(ctx, p, queryDone) queryDone = nil diff --git a/pkg/sql/conn_executor_prepare.go b/pkg/sql/conn_executor_prepare.go index a3c2e9c4c1e7..8cd9e24305e4 100644 --- a/pkg/sql/conn_executor_prepare.go +++ b/pkg/sql/conn_executor_prepare.go @@ -190,12 +190,6 @@ func (ex *connExecutor) populatePrepared( prepared := stmt.Prepared p.extendedEvalCtx.PrepareOnly = true - p.extendedEvalCtx.ActiveMemAcc = &prepared.memAcc - // constantMemAcc accounts for all constant folded values that are computed - // prior to any rows being computed. - constantMemAcc := p.extendedEvalCtx.Mon.MakeBoundAccount() - p.extendedEvalCtx.ActiveMemAcc = &constantMemAcc - defer constantMemAcc.Close(ctx) protoTS, err := p.isAsOf(stmt.AST, ex.server.cfg.Clock.Now() /* max */) if err != nil { diff --git a/pkg/sql/distsql_running.go b/pkg/sql/distsql_running.go index 639f460a01c2..c58e3f77e770 100644 --- a/pkg/sql/distsql_running.go +++ b/pkg/sql/distsql_running.go @@ -659,8 +659,6 @@ func (dsp *DistSQLPlanner) planAndRunSubquery( subqueryMemAccount := subqueryMonitor.MakeBoundAccount() defer subqueryMemAccount.Close(ctx) - evalCtx.ActiveMemAcc = &subqueryMemAccount - var subqueryPlanCtx *PlanningCtx var distributeSubquery bool if maybeDistribute { diff --git a/pkg/sql/distsqlrun/flow.go b/pkg/sql/distsqlrun/flow.go index e4ad811f4c01..9a9002c4849f 100644 --- a/pkg/sql/distsqlrun/flow.go +++ b/pkg/sql/distsqlrun/flow.go @@ -250,22 +250,6 @@ func (f *Flow) setupInboundStream( return nil } -// This RowReceiver clears its BoundAccount on every input row. This is useful -// for clearing the per-row memory account that's used for expression -// evaluation. -type accountClearingRowReceiver struct { - RowReceiver - ctx context.Context - acc *mon.BoundAccount -} - -func (r *accountClearingRowReceiver) Push( - row sqlbase.EncDatumRow, meta *ProducerMetadata, -) ConsumerStatus { - r.acc.Clear(r.ctx) - return r.RowReceiver.Push(row, meta) -} - // setupOutboundStream sets up an output stream; if the stream is local, the // RowChannel is looked up in the localStreams map; otherwise an outgoing // mailbox is created. @@ -273,13 +257,7 @@ func (f *Flow) setupOutboundStream(spec distsqlpb.StreamEndpointSpec) (RowReceiv sid := spec.StreamID switch spec.Type { case distsqlpb.StreamEndpointSpec_SYNC_RESPONSE: - // Wrap the syncFlowConsumer in a row receiver that clears the row's memory - // account. - return &accountClearingRowReceiver{ - acc: f.EvalCtx.ActiveMemAcc, - ctx: f.EvalCtx.Ctx(), - RowReceiver: f.syncFlowConsumer, - }, nil + return f.syncFlowConsumer, nil case distsqlpb.StreamEndpointSpec_REMOTE: outbox := newOutbox(&f.FlowCtx, spec.TargetNodeID, f.id, sid) @@ -373,10 +351,6 @@ func (f *Flow) makeProcessor( // Initialize any routers (the setupRouter case above) and outboxes. types := proc.OutputTypes() rowRecv := output.(*copyingRowReceiver).RowReceiver - clearer, ok := rowRecv.(*accountClearingRowReceiver) - if ok { - rowRecv = clearer.RowReceiver - } switch o := rowRecv.(type) { case router: o.init(ctx, &f.FlowCtx, types) @@ -687,8 +661,7 @@ func (f *Flow) Cleanup(ctx context.Context) { if f.status == FlowFinished { panic("flow cleanup called twice") } - // This closes the account and monitor opened in ServerImpl.setupFlow. - f.EvalCtx.ActiveMemAcc.Close(ctx) + // This closes the monitor opened in ServerImpl.setupFlow. f.EvalCtx.Stop(ctx) for _, p := range f.processors { if d, ok := p.(Releasable); ok { diff --git a/pkg/sql/distsqlrun/server.go b/pkg/sql/distsqlrun/server.go index 42f6290d622f..db5df0e2dbc9 100644 --- a/pkg/sql/distsqlrun/server.go +++ b/pkg/sql/distsqlrun/server.go @@ -320,7 +320,7 @@ func (ds *ServerImpl) setupFlow( // sp will be Finish()ed by Flow.Cleanup(). ctx = opentracing.ContextWithSpan(ctx, sp) - // The monitor and account opened here are closed in Flow.Cleanup(). + // The monitor opened here are closed in Flow.Cleanup(). monitor := mon.MakeMonitor( "flow", mon.MemoryResource, @@ -331,7 +331,6 @@ func (ds *ServerImpl) setupFlow( ds.Settings, ) monitor.Start(ctx, parentMonitor, mon.BoundAccount{}) - acc := monitor.MakeBoundAccount() // Figure out what txn the flow needs to run in, if any. // For local flows, the txn comes from localState.Txn. For non-local ones, we @@ -355,7 +354,6 @@ func (ds *ServerImpl) setupFlow( if localState.EvalContext != nil { evalCtx = localState.EvalContext evalCtx.Mon = &monitor - evalCtx.ActiveMemAcc = &acc evalCtx.Txn = txn } else { location, err := timeutil.TimeZoneStringToLocation(req.EvalContext.Location) @@ -403,13 +401,12 @@ func (ds *ServerImpl) setupFlow( evalPlanner := &sqlbase.DummyEvalPlanner{} sequence := &sqlbase.DummySequenceOperators{} evalCtx = &tree.EvalContext{ - Settings: ds.ServerConfig.Settings, - SessionData: sd, - ClusterID: ds.ServerConfig.ClusterID.Get(), - NodeID: nodeID, - ReCache: ds.regexpCache, - Mon: &monitor, - ActiveMemAcc: &acc, + Settings: ds.ServerConfig.Settings, + SessionData: sd, + ClusterID: ds.ServerConfig.ClusterID.Get(), + NodeID: nodeID, + ReCache: ds.regexpCache, + Mon: &monitor, // TODO(andrei): This is wrong. Each processor should override Ctx with its // own context. Context: ctx, diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 5dc1f4defe5a..80871770ab4d 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -489,11 +489,6 @@ func (dc *databaseCacheHolder) updateSystemConfig(cfg *config.SystemConfig) { func forEachRow(params runParams, p planNode, f func(tree.Datums) error) error { next, err := p.Next(params) for ; next; next, err = p.Next(params) { - // If we're tracking memory, clear the previous row's memory account. - if params.extendedEvalCtx.ActiveMemAcc != nil { - params.extendedEvalCtx.ActiveMemAcc.Clear(params.ctx) - } - if err := f(p.Values()); err != nil { return err } diff --git a/pkg/sql/logictest/testdata/logic_test/builtin_function b/pkg/sql/logictest/testdata/logic_test/builtin_function index b29c986490c2..aff8f0225013 100644 --- a/pkg/sql/logictest/testdata/logic_test/builtin_function +++ b/pkg/sql/logictest/testdata/logic_test/builtin_function @@ -262,7 +262,7 @@ SELECT repeat('Pg', -1) || 'empty' ---- empty -statement error pq: repeat\(\): .* memory budget exceeded +statement error pq: repeat\(\): requested length too large SELECT repeat('s', 9223372036854775807) # Regression for #19035. @@ -2012,10 +2012,10 @@ SELECT lpad('abc', 5, ''), rpad('abc', 5, '') ---- abc abc -query error memory budget exceeded +query error requested length too large SELECT lpad('abc', 100000000000000) -query error memory budget exceeded +query error requested length too large SELECT rpad('abc', 100000000000000) query TT diff --git a/pkg/sql/plan_node_to_row_source.go b/pkg/sql/plan_node_to_row_source.go index 2891d696a312..32f1095747de 100644 --- a/pkg/sql/plan_node_to_row_source.go +++ b/pkg/sql/plan_node_to_row_source.go @@ -161,10 +161,6 @@ func (p *planNodeToRowSource) Next() (sqlbase.EncDatumRow, *distsqlrun.ProducerM // by Nexting our source until exhaustion. next, err := p.node.Next(p.params) for ; next; next, err = p.node.Next(p.params) { - // If we're tracking memory, clear the previous row's memory account. - if p.params.extendedEvalCtx.ActiveMemAcc != nil { - p.params.extendedEvalCtx.ActiveMemAcc.Clear(p.params.ctx) - } count++ } if err != nil { diff --git a/pkg/sql/plan_spans.go b/pkg/sql/plan_spans.go index 653b262e0f19..8d25e9c8fd8f 100644 --- a/pkg/sql/plan_spans.go +++ b/pkg/sql/plan_spans.go @@ -155,7 +155,6 @@ func insertNodeWithValuesSpans( // completed, so that the values don't need to be computed again during // plan execution. rowAcc := params.extendedEvalCtx.Mon.MakeBoundAccount() - params.extendedEvalCtx.ActiveMemAcc = &rowAcc defer rowAcc.Close(params.ctx) defer v.Reset(params.ctx) diff --git a/pkg/sql/planner.go b/pkg/sql/planner.go index ed9714bf3d4b..e99e9f41fe84 100644 --- a/pkg/sql/planner.go +++ b/pkg/sql/planner.go @@ -265,15 +265,11 @@ func newInternalPlanner( p.extendedEvalCtx.Placeholders = &p.semaCtx.Placeholders p.extendedEvalCtx.Tables = tables - acc := plannerMon.MakeBoundAccount() - p.extendedEvalCtx.ActiveMemAcc = &acc - p.queryCacheSession.Init() return p, func() { // Note that we capture ctx here. This is only valid as long as we create // the context as explained at the top of the method. - acc.Close(ctx) plannerMon.Stop(ctx) } } @@ -556,8 +552,11 @@ func (p *planner) runWithDistSQL( columns := planColumns(plan) // Initialize a row container for the DistSQL execution engine to write into. + // The caller of this method will call Close on the returned RowContainer, + // which will close this account. + acc := planCtx.EvalContext().Mon.MakeBoundAccount() ci := sqlbase.ColTypeInfoFromResCols(columns) - rows := rowcontainer.NewRowContainer(*p.extendedEvalCtx.ActiveMemAcc, ci, 0 /* rowCapacity */) + rows := rowcontainer.NewRowContainer(acc, ci, 0 /* rowCapacity */) rowResultWriter := NewRowResultWriter(rows) recv := MakeDistSQLReceiver( ctx, diff --git a/pkg/sql/scrub.go b/pkg/sql/scrub.go index b616f58b56c3..666cc0c8744e 100644 --- a/pkg/sql/scrub.go +++ b/pkg/sql/scrub.go @@ -546,7 +546,8 @@ func scrubRunDistSQL( columnTypes []sqlbase.ColumnType, ) (*rowcontainer.RowContainer, error) { ci := sqlbase.ColTypeInfoFromColTypes(columnTypes) - rows := rowcontainer.NewRowContainer(*p.extendedEvalCtx.ActiveMemAcc, ci, 0 /* rowCapacity */) + acc := p.extendedEvalCtx.Mon.MakeBoundAccount() + rows := rowcontainer.NewRowContainer(acc, ci, 0 /* rowCapacity */) rowResultWriter := NewRowResultWriter(rows) recv := MakeDistSQLReceiver( ctx, diff --git a/pkg/sql/sem/builtins/builtins.go b/pkg/sql/sem/builtins/builtins.go index 6c3a49bc780c..c79f00934dae 100644 --- a/pkg/sql/sem/builtins/builtins.go +++ b/pkg/sql/sem/builtins/builtins.go @@ -48,6 +48,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/sql/sqlbase" "github.com/cockroachdb/cockroach/pkg/util/duration" + "github.com/cockroachdb/cockroach/pkg/util/humanizeutil" "github.com/cockroachdb/cockroach/pkg/util/ipaddr" "github.com/cockroachdb/cockroach/pkg/util/json" "github.com/cockroachdb/cockroach/pkg/util/log" @@ -69,8 +70,12 @@ var ( errChrValueTooSmall = pgerror.NewError(pgerror.CodeInvalidParameterValueError, "input value must be >= 0") errChrValueTooLarge = pgerror.NewErrorf(pgerror.CodeInvalidParameterValueError, "input value must be <= %d (maximum Unicode code point)", utf8.MaxRune) + errStringTooLarge = pgerror.NewErrorf(pgerror.CodeProgramLimitExceededError, + fmt.Sprintf("requested length too large, exceeds %s", humanizeutil.IBytes(maxAllocatedStringSize))) ) +const maxAllocatedStringSize = 128 * 1024 * 1024 + const errInsufficientArgsFmtString = "unknown signature: %s()" const ( @@ -181,17 +186,11 @@ var builtins = map[string]builtinDefinition{ "lower": makeBuiltin(tree.FunctionProperties{Category: categoryString}, stringOverload1(func(evalCtx *tree.EvalContext, s string) (tree.Datum, error) { - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(s))); err != nil { - return nil, err - } return tree.NewDString(strings.ToLower(s)), nil }, types.String, "Converts all characters in `val` to their lower-case equivalents.")), "upper": makeBuiltin(tree.FunctionProperties{Category: categoryString}, stringOverload1(func(evalCtx *tree.EvalContext, s string) (tree.Datum, error) { - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(s))); err != nil { - return nil, err - } return tree.NewDString(strings.ToUpper(s)), nil }, types.String, "Converts all characters in `val` to their to their upper-case equivalents.")), @@ -206,13 +205,14 @@ var builtins = map[string]builtinDefinition{ ReturnType: tree.FixedReturnType(types.String), Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { var buffer bytes.Buffer + length := 0 for _, d := range args { if d == tree.DNull { continue } - nextLength := len(string(tree.MustBeDString(d))) - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(nextLength)); err != nil { - return nil, err + length += len(string(tree.MustBeDString(d))) + if length > maxAllocatedStringSize { + return nil, errStringTooLarge } buffer.WriteString(string(tree.MustBeDString(d))) } @@ -236,14 +236,14 @@ var builtins = map[string]builtinDefinition{ sep := string(tree.MustBeDString(args[0])) var buf bytes.Buffer prefix := "" + length := 0 for _, d := range args[1:] { if d == tree.DNull { continue } - nextLength := len(prefix) + len(string(tree.MustBeDString(d))) - if err := evalCtx.ActiveMemAcc.Grow( - evalCtx.Ctx(), int64(nextLength)); err != nil { - return nil, err + length += len(prefix) + len(string(tree.MustBeDString(d))) + if length > maxAllocatedStringSize { + return nil, errStringTooLarge } // Note: we can't use the range index here because that // would break when the 2nd argument is NULL. @@ -677,14 +677,11 @@ var builtins = map[string]builtinDefinition{ count = 0 } else if ln/count != len(s) { // Detect overflow and trigger an error. - return nil, pgerror.NewError( - pgerror.CodeProgramLimitExceededError, "requested length too large", - ) + return nil, errStringTooLarge + } else if ln > maxAllocatedStringSize { + return nil, errStringTooLarge } - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(ln)); err != nil { - return nil, err - } return tree.NewDString(strings.Repeat(s, count)), nil }, Info: "Concatenates `input` `repeat_counter` number of times.\n\nFor example, " + @@ -922,10 +919,7 @@ var builtins = map[string]builtinDefinition{ Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { s := string(tree.MustBeDString(args[0])) length := int(tree.MustBeDInt(args[1])) - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(length)); err != nil { - return nil, err - } - ret, err := lpad(evalCtx, s, length, " ") + ret, err := lpad(s, length, " ") if err != nil { return nil, err } @@ -941,7 +935,7 @@ var builtins = map[string]builtinDefinition{ s := string(tree.MustBeDString(args[0])) length := int(tree.MustBeDInt(args[1])) fill := string(tree.MustBeDString(args[2])) - ret, err := lpad(evalCtx, s, length, fill) + ret, err := lpad(s, length, fill) if err != nil { return nil, err } @@ -960,7 +954,7 @@ var builtins = map[string]builtinDefinition{ Fn: func(evalCtx *tree.EvalContext, args tree.Datums) (tree.Datum, error) { s := string(tree.MustBeDString(args[0])) length := int(tree.MustBeDInt(args[1])) - ret, err := rpad(evalCtx, s, length, " ") + ret, err := rpad(s, length, " ") if err != nil { return nil, err } @@ -976,7 +970,7 @@ var builtins = map[string]builtinDefinition{ s := string(tree.MustBeDString(args[0])) length := int(tree.MustBeDInt(args[1])) fill := string(tree.MustBeDString(args[2])) - ret, err := rpad(evalCtx, s, length, fill) + ret, err := rpad(s, length, fill) if err != nil { return nil, err } @@ -1025,8 +1019,8 @@ var builtins = map[string]builtinDefinition{ "reverse": makeBuiltin(defProps(), stringOverload1(func(evalCtx *tree.EvalContext, s string) (tree.Datum, error) { - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(s))); err != nil { - return nil, err + if len(s) > maxAllocatedStringSize { + return nil, errStringTooLarge } runes := []rune(s) for i, j := 0, len(runes)-1; i < j; i, j = i+1, j-1 { @@ -1050,13 +1044,10 @@ var builtins = map[string]builtinDefinition{ // Largest result is if there are no replacements. maxResultLen = int64(len(input)) } - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), maxResultLen); err != nil { - return nil, err + if maxResultLen > maxAllocatedStringSize { + return nil, errStringTooLarge } result := strings.Replace(input, from, to, -1) - if err := evalCtx.ActiveMemAcc.Resize(evalCtx.Ctx(), maxResultLen, int64(len(result))); err != nil { - return nil, err - } return tree.NewDString(result), nil }, types.String, @@ -1066,9 +1057,6 @@ var builtins = map[string]builtinDefinition{ "translate": makeBuiltin(defProps(), stringOverload3("input", "find", "replace", func(evalCtx *tree.EvalContext, s, from, to string) (tree.Datum, error) { - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(s))); err != nil { - return nil, err - } const deletionRune = utf8.MaxRune + 1 translation := make(map[rune]rune, len(from)) for _, fromRune := range from { @@ -1125,9 +1113,6 @@ var builtins = map[string]builtinDefinition{ if err != nil { return nil, err } - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(string(tree.MustBeDString(result))))); err != nil { - return nil, err - } return result, nil }, Info: "Replaces matches for the Regular Expression `regex` in `input` with the " + @@ -1150,9 +1135,6 @@ var builtins = map[string]builtinDefinition{ if err != nil { return nil, err } - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(string(tree.MustBeDString(result))))); err != nil { - return nil, err - } return result, nil }, Info: "Replaces matches for the regular expression `regex` in `input` with the regular " + @@ -1256,9 +1238,6 @@ CockroachDB supports the following flags: "initcap": makeBuiltin(defProps(), stringOverload1(func(evalCtx *tree.EvalContext, s string) (tree.Datum, error) { - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(len(s))); err != nil { - return nil, err - } return tree.NewDString(strings.Title(strings.ToLower(s))), nil }, types.String, "Capitalizes the first letter of `val`.")), @@ -4456,14 +4435,14 @@ func padMaybeTruncate(s string, length int, fill string) (ok bool, slen int, ret return false, slen, s } -func lpad(evalCtx *tree.EvalContext, s string, length int, fill string) (string, error) { +func lpad(s string, length int, fill string) (string, error) { + if length > maxAllocatedStringSize { + return "", errStringTooLarge + } ok, slen, ret := padMaybeTruncate(s, length, fill) if ok { return ret, nil } - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(length)); err != nil { - return "", err - } var buf strings.Builder fillRunes := []rune(fill) for i := 0; i < length-slen; i++ { @@ -4474,14 +4453,14 @@ func lpad(evalCtx *tree.EvalContext, s string, length int, fill string) (string, return buf.String(), nil } -func rpad(evalCtx *tree.EvalContext, s string, length int, fill string) (string, error) { +func rpad(s string, length int, fill string) (string, error) { + if length > maxAllocatedStringSize { + return "", errStringTooLarge + } ok, slen, ret := padMaybeTruncate(s, length, fill) if ok { return ret, nil } - if err := evalCtx.ActiveMemAcc.Grow(evalCtx.Ctx(), int64(length)); err != nil { - return "", err - } var buf strings.Builder buf.WriteString(s) fillRunes := []rune(fill) diff --git a/pkg/sql/sem/builtins/builtins_test.go b/pkg/sql/sem/builtins/builtins_test.go index 915da7b66f1f..2dd33b823dab 100644 --- a/pkg/sql/sem/builtins/builtins_test.go +++ b/pkg/sql/sem/builtins/builtins_test.go @@ -192,7 +192,7 @@ func TestEscapeFormatRandom(t *testing.T) { func TestLPadRPad(t *testing.T) { testCases := []struct { - padFn func(*tree.EvalContext, string, int, string) (string, error) + padFn func(string, int, string) (string, error) str string length int fill string @@ -228,9 +228,8 @@ func TestLPadRPad(t *testing.T) { {rpad, "Hello", 8, "世界", "Hello世界世"}, {rpad, "foo", -1, "世界", ""}, } - evalCtx := tree.NewTestingEvalContext(cluster.MakeTestingClusterSettings()) for _, tc := range testCases { - out, err := tc.padFn(evalCtx, tc.str, tc.length, tc.fill) + out, err := tc.padFn(tc.str, tc.length, tc.fill) if err != nil { t.Errorf("Found err %v, expected nil", err) } diff --git a/pkg/sql/sem/tree/constant_eval_test.go b/pkg/sql/sem/tree/constant_eval_test.go index 86333da09709..83296f344f0c 100644 --- a/pkg/sql/sem/tree/constant_eval_test.go +++ b/pkg/sql/sem/tree/constant_eval_test.go @@ -41,7 +41,6 @@ func TestConstantEvalArrayComparison(t *testing.T) { ctx := tree.NewTestingEvalContext(cluster.MakeTestingClusterSettings()) defer ctx.Mon.Stop(context.Background()) - defer ctx.ActiveMemAcc.Close(context.Background()) c := tree.MakeConstantEvalVisitor(ctx) expr, _ = tree.WalkExpr(&c, typedExpr) if err := c.Err(); err != nil { diff --git a/pkg/sql/sem/tree/eval.go b/pkg/sql/sem/tree/eval.go index 5d6a6e89f2e8..93a10335d65f 100644 --- a/pkg/sql/sem/tree/eval.go +++ b/pkg/sql/sem/tree/eval.go @@ -2514,11 +2514,6 @@ type EvalContext struct { TestingKnobs EvalContextTestingKnobs Mon *mon.BytesMonitor - - // ActiveMemAcc is the account to which values are allocated during - // evaluation. It can change over the course of evaluation, such as on a - // per-row basis. - ActiveMemAcc *mon.BoundAccount } // MakeTestingEvalContext returns an EvalContext that includes a MemoryMonitor. @@ -2547,8 +2542,6 @@ func MakeTestingEvalContextWithMon(st *cluster.Settings, monitor *mon.BytesMonit monitor.Start(context.Background(), nil /* pool */, mon.MakeStandaloneBudget(math.MaxInt64)) ctx.Mon = monitor ctx.Context = context.TODO() - acc := monitor.MakeBoundAccount() - ctx.ActiveMemAcc = &acc now := timeutil.Now() ctx.SetTxnTimestamp(now) ctx.SetStmtTimestamp(now) diff --git a/pkg/sql/sem/tree/eval_test.go b/pkg/sql/sem/tree/eval_test.go index 3735c795a61a..aa2142700c0c 100644 --- a/pkg/sql/sem/tree/eval_test.go +++ b/pkg/sql/sem/tree/eval_test.go @@ -36,10 +36,7 @@ import ( func TestEval(t *testing.T) { ctx := tree.NewTestingEvalContext(cluster.MakeTestingClusterSettings()) - // We have to manually close this account because we're doing the evaluations - // ourselves. - defer ctx.Mon.Stop(context.Background()) - defer ctx.ActiveMemAcc.Close(context.Background()) + defer ctx.Stop(context.Background()) walk := func(t *testing.T, getExpr func(tree.TypedExpr) (tree.TypedExpr, error)) { datadriven.Walk(t, filepath.Join("testdata", "eval"), func(t *testing.T, path string) { diff --git a/pkg/sql/sem/tree/normalize_test.go b/pkg/sql/sem/tree/normalize_test.go index e81eb6892d78..6e6b3ad1d308 100644 --- a/pkg/sql/sem/tree/normalize_test.go +++ b/pkg/sql/sem/tree/normalize_test.go @@ -289,7 +289,6 @@ func TestNormalizeExpr(t *testing.T) { rOrig := typedExpr.String() ctx := tree.NewTestingEvalContext(cluster.MakeTestingClusterSettings()) defer ctx.Mon.Stop(context.Background()) - defer ctx.ActiveMemAcc.Close(context.Background()) r, err := ctx.NormalizeExpr(typedExpr) if err != nil { t.Fatalf("%s: %v", d.expr, err)