Merge #99887

99887: distsql: fix sql.mem.distsql.current metric r=yuzefovich a=yuzefovich

**sql: don't create unused monitor and account**

As of 961e66f we no longer need to
create memory monitors and accounts for sub- and post-queries so this
commit removes them.

Release note: None

**util/mon: remove redundant locking in NewMonitorInheritWithLimit**

This locking is no longer needed as of 472e740.

Release note: None

**distsql: fix sql.mem.distsql.current metric**

This commit fixes double counting of remote DistSQL flows' memory usage
against `sql.mem.distsql.current` metric. This was the case since we
passed the metric to both the `flow` monitor (created for all flows,
both local and remote) and the `distsql` monitor (which tracks the
memory usage of all remote flows). This is now fixed by only passing the
metric in the former case.

Fixes: #91787.

Release note (bug fix): Previously, `sql.mem.distsql.current` metric
would double count the memory usage of remote DistSQL flows and this has
been fixed.

Co-authored-by: Yahor Yuzefovich <[email protected]>

pkg/sql/buffer_util.go

@@ -81,6 +81,8 @@ func (c *rowContainerHelper) initMonitors(
 	ctx context.Context, evalContext *extendedEvalContext, opName redact.RedactableString,
 ) {
 	distSQLCfg := &evalContext.DistSQLPlanner.distSQLSrv.ServerConfig
+	// TODO(yuzefovich): currently the memory usage of c.memMonitor doesn't
+	// count against sql.mem.distsql.current metric. Fix it.
 	c.memMonitor = execinfra.NewLimitedMonitorNoFlowCtx(
 		ctx, evalContext.Planner.Mon(), distSQLCfg, evalContext.SessionData(),
 		redact.Sprintf("%s-limited", opName),

pkg/sql/distsql/server.go

@@ -85,9 +85,12 @@ func NewServer(
 		memMonitor: mon.NewMonitor(
 			"distsql",
 			mon.MemoryResource,
-			cfg.Metrics.CurBytesCount,
-			cfg.Metrics.MaxBytesHist,
-			-1, /* increment: use default block size */
+			// Note that we don't use 'sql.mem.distsql.*' metrics here since
+			// that would double count them with the 'flow' monitor in
+			// setupFlow.
+			nil, /* curCount */
+			nil, /* maxHist */
+			-1,  /* increment: use default block size */
 			noteworthyMemoryUsageBytes,
 			cfg.Settings,
 		),

pkg/sql/distsql_running.go

@@ -1706,21 +1706,6 @@ func (dsp *DistSQLPlanner) planAndRunSubquery(
 	skipDistSQLDiagramGeneration bool,
 	mustUseLeafTxn bool,
 ) error {
-	subqueryMonitor := mon.NewMonitor(
-		"subquery",
-		mon.MemoryResource,
-		dsp.distSQLSrv.Metrics.CurBytesCount,
-		dsp.distSQLSrv.Metrics.MaxBytesHist,
-		-1, /* use default block size */
-		noteworthyMemoryUsageBytes,
-		dsp.distSQLSrv.Settings,
-	)
-	subqueryMonitor.StartNoReserved(ctx, evalCtx.Planner.Mon())
-	defer subqueryMonitor.Stop(ctx)
-
-	subqueryMemAccount := subqueryMonitor.MakeBoundAccount()
-	defer subqueryMemAccount.Close(ctx)
-
 	distributeSubquery := getPlanDistribution(
 		ctx, planner.Descriptors().HasUncommittedTypes(),
 		planner.SessionData().DistSQLMode, subqueryPlan.plan,
@@ -2109,21 +2094,6 @@ func (dsp *DistSQLPlanner) planAndRunPostquery(
 	associateNodeWithComponents func(exec.Node, execComponents),
 	addTopLevelQueryStats func(stats *topLevelQueryStats),
 ) error {
-	postqueryMonitor := mon.NewMonitor(
-		"postquery",
-		mon.MemoryResource,
-		dsp.distSQLSrv.Metrics.CurBytesCount,
-		dsp.distSQLSrv.Metrics.MaxBytesHist,
-		-1, /* use default block size */
-		noteworthyMemoryUsageBytes,
-		dsp.distSQLSrv.Settings,
-	)
-	postqueryMonitor.StartNoReserved(ctx, planner.Mon())
-	defer postqueryMonitor.Stop(ctx)
-
-	postqueryMemAccount := postqueryMonitor.MakeBoundAccount()
-	defer postqueryMemAccount.Close(ctx)
-
 	distributePostquery := getPlanDistribution(
 		ctx, planner.Descriptors().HasUncommittedTypes(),
 		planner.SessionData().DistSQLMode, postqueryPlan,

pkg/sql/sem/eval/deps.go

@@ -195,6 +195,10 @@ type Planner interface {
 	tree.FunctionReferenceResolver
 
 	// Mon returns the Planner's monitor.
+	//
+	// TODO(yuzefovich): memory usage against this monitor doesn't count against
+	// sql.mem.distsql.current metric, audit the callers to see whether this is
+	// undesirable in some places.
 	Mon() *mon.BytesMonitor
 
 	// ExecutorConfig returns *ExecutorConfig

pkg/sql/tablewriter_upsert_opt.go

@@ -105,7 +105,7 @@ func (tu *optTableUpserter) init(
 		return err
 	}
 
-	// rowsNeeded, set upon initialization, indicates pkg/sql/backfill.gowhether or not we want
+	// rowsNeeded, set upon initialization, indicates whether or not we want
 	// rows returned from the operation.
 	if tu.rowsNeeded {
 		tu.resultRow = make(tree.Datums, len(tu.returnCols))

pkg/util/mon/bytes_usage.go

@@ -436,8 +436,6 @@ func NewMonitorWithLimit(
 func NewMonitorInheritWithLimit(
 	name redact.RedactableString, limit int64, m *BytesMonitor,
 ) *BytesMonitor {
-	m.mu.Lock()
-	defer m.mu.Unlock()
 	return NewMonitorWithLimit(
 		name,
 		m.resource,