From 2b00f15376c8eb486770fd39f71566054800f134 Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Thu, 14 Feb 2019 17:58:41 -0500 Subject: [PATCH] sql: remove EvalContext.ActiveMemAcc This memory account was used to track allocations performed by builtins that allocated memory, mainly string manipulation builtins. It was pretty hard to use this correctly, as the lifetimes of the account were never particularly clear. There were a couple of hacks in place to try to clear the memory at the right times, but they didn't work correctly, leading to crashes that were hard to diagnose. Rather than try to make this work perfectly, this commit removes the memory accounting for these builtins and replaces it with a hard cap on the size of string allocations - set arbitrarily to 64 MB at this time. Release note: None --- pkg/sql/builtin_mem_usage_test.go | 103 +----------------- pkg/sql/conn_executor_exec.go | 6 - pkg/sql/conn_executor_prepare.go | 6 - pkg/sql/distsql_running.go | 2 - pkg/sql/distsqlrun/flow.go | 31 +----- pkg/sql/distsqlrun/server.go | 17 ++- pkg/sql/exec_util.go | 5 - .../testdata/logic_test/builtin_function | 6 +- pkg/sql/plan_node_to_row_source.go | 4 - pkg/sql/plan_spans.go | 1 - pkg/sql/planner.go | 9 +- pkg/sql/scrub.go | 3 +- pkg/sql/sem/builtins/builtins.go | 85 ++++++--------- pkg/sql/sem/builtins/builtins_test.go | 5 +- pkg/sql/sem/tree/constant_eval_test.go | 1 - pkg/sql/sem/tree/eval.go | 7 -- pkg/sql/sem/tree/eval_test.go | 5 +- pkg/sql/sem/tree/normalize_test.go | 1 - 18 files changed, 56 insertions(+), 241 deletions(-) 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)