haven't looked at any of the clients changed here yet. Let's do one more iteration on the main classes.

Review status: 0 of 22 files reviewed at latest revision, 24 unresolved discussions, some commit checks failed.

cli/flags.go, line 329 at r1 (raw file):

      sqlSize = newBytesValue(&serverCtx.SQLMemoryPoolSize)
      varFlag(f, sqlSize, cliflags.SQLMem)

how are you thinking of flags vs env vars?

cli/start.go, line 117 at r1 (raw file):

      ctx.CacheSize = size / 4

      // Default the SQL memory pool size to 1/4 of total memory.  Again

two space before "again"

sql/mon/mem_pool.go, line 35 at r1 (raw file):

  mu struct {
      syncutil.Mutex
      cur int64

add unit here (and I'd add it throughout all params in this file, but I feel most strongly about this one).

sql/mon/mem_pool.go, line 51 at r1 (raw file):

  defer p.mu.Unlock()

  if p.mu.cur > p.max-extraSize {

nit: to me, if p.mu.cur + extrSize > p.max reads better

sql/mon/mem_pool.go, line 52 at r1 (raw file):

  if p.mu.cur > p.max-extraSize {
      err := errors.Errorf("memory budget exceeded: %d requested, %s already allocated",

%d bytes

sql/mon/mem_pool.go, line 52 at r1 (raw file):

  if p.mu.cur > p.max-extraSize {
      err := errors.Errorf("memory budget exceeded: %d requested, %s already allocated",

IMHO, I think for the purposes of this error msg, it's less interesting the amount that's been allocated in the pool, and more interesting what's been already allocated in the acc.

sql/mon/mem_pool.go, line 56 at r1 (raw file):

          humanizeutil.IBytes(acc.curAllocated))
      if log.V(2) {
          log.Errorf(ctx, "%s, %s global, %s global max",

is there a VErrorf() ?

sql/mon/mem_usage.go, line 30 at r1 (raw file):

)

// MemoryUsageMonitor defines an object that can track and limit

the "limit" part is wrong, correct (I think it was theoretically doing that before this PR, but not any more?)? And in fact the "tracking" is not really the point of this struct either, is it? It's all about buffering. Maybe we should rename it to... PoolProxy? MemoryBuffer?
Should it share an interface with the Pool?

sql/mon/mem_usage.go, line 38 at r1 (raw file):

// and after a processing context.
// The various counters express sizes in bytes.
type MemoryUsageMonitor struct {

As discussed, let's put a much bigger comment here explaining the interactions between Pool - Monitor - Acccount (and probably mention WrappedAccount too)

sql/mon/mem_usage.go, line 40 at r1 (raw file):

type MemoryUsageMonitor struct {
  // curAllocated tracks the current amount of memory allocated for
  // in-memory row storage.

delete the reference to "in-memory row storage".

sql/mon/mem_usage.go, line 44 at r1 (raw file):

  // totalAllocated tracks the cumulated amount of memory allocated.
  totalAllocated int64

is this member actually useful for anything? I'd vote to remove it, particularly since the comment didn't make it clear to me about what it means and I had to look it up.

sql/mon/mem_usage.go, line 46 at r1 (raw file):

  totalAllocated int64

  // maxAllocated tracks the high water mark of allocations.

maybe suggest in this comment how this is going to be used. Otherwise it seems superfluous to me.

sql/mon/mem_usage.go, line 50 at r1 (raw file):

  // curBudget sets the allowable upper limit for allocations,
  // representing the budget pre-allocated at the pool.

is this comment correct? What does "pre-allocated" mean?

sql/mon/mem_usage.go, line 53 at r1 (raw file):

  curBudget MemoryAccount

  // preBudget indicates how much memory was already reserved for this

maybe expand this comment a bit. Why has any memory been reserved before this was instantiated? Why isn't that memory just set as the initial curBudget?

later: having discussed some, I understand what the deal is. I suggest renaming the field and changing the comment - instead of "already reserved...", say that this is the amount of memory that's never released (maybe call it reserve).

sql/mon/mem_usage.go, line 59 at r1 (raw file):

  // pool specifies where to send requests to increase or decrease
  // curBudget.
  pool *MemoryPool

make this the first member (to highlight the parent-child structure, and also to celebrate the fact that you've finally embedded a pointer into a struct :) )

sql/mon/mem_usage.go, line 70 at r1 (raw file):

var noteworthyMemoryUsageBytes = envutil.EnvOrDefaultInt64("COCKROACH_NOTEWORTHY_MEMORY_USAGE", 10000)

// allocHysteresisFactor determines the maximum difference between the

if we call this "hysterisis", I say you rename the "noteworthy" above to "hysterical"

sql/mon/mem_usage.go, line 73 at r1 (raw file):

// amount of memory used by a monitor and the amount of memory
// reserved at the upstream pool before the monitor relinquishes the
// memory back to the pool.  This is useful so that a monitor

double space

sql/mon/mem_usage.go, line 76 at r1 (raw file):

// currently at the boundary of a block does not cause contention when
// accounts cause its allocation counter to grow and shrink slightly
// beyond and beneath an allocation block boundary.  The difference is

double space

sql/mon/mem_usage.go, line 78 at r1 (raw file):

// beyond and beneath an allocation block boundary.  The difference is
// expressed as a number of blocks of size `poolAllocationSize`.
var allocHysteresisFactor = envutil.EnvOrDefaultInt("COCKROACH_MEMORY_FREE_FACTOR", 10)

if it's expressed in "blocks", maybe "factor" is not the right name. "Buffer"?

sql/mon/mem_usage.go, line 95 at r1 (raw file):

  minExtra = mm.roundSize(minExtra)
  if log.V(2) {
      log.Infof(ctx, "requesting %d bytes from the pool", minExtra)

VInfof() here and throughout

sql/mon/mem_usage.go, line 102 at r1 (raw file):

// releaseBudget relinquishes all the monitor's memory back to the
// pool.
func (mm *MemoryUsageMonitor) releaseBudget(ctx context.Context) {

mmm what about the preBudget? That doesn't get released?
Does this need to be a different function from StopMonitor()?

sql/mon/mem_usage.go, line 164 at r1 (raw file):

// releaseMemory releases memory previously successfully registered
// via reserveMemory().
func (mm *MemoryUsageMonitor) releaseMemory(ctx context.Context, x int64) {

rename x to smth else

sql/mon/mem_usage_mutex.go, line 24 at r1 (raw file):

)

// MemoryUsageMonitorWithMutex encapsulates a MemoryUsageMonitor using

just say it's a thread-safe Monitor. And in the comments of the regular Monitor, say it's not thread-safe and link to this one.

sql/mon/mem_usage_mutex.go, line 26 at r1 (raw file):

// MemoryUsageMonitorWithMutex encapsulates a MemoryUsageMonitor using
// a Mutex.
type MemoryUsageMonitorWithMutex struct {

Do we really need this class? I'd just put a mutex in the other one and call it a thread-safe day. If the mutex is not contended, it shouldn't cost much.

Comments from Reviewable

I'm processing these comments bit by bit. Here's a first batch, gimme more time before I address the bigger ones.

Review status: 0 of 22 files reviewed at latest revision, 24 unresolved discussions, some commit checks pending.

cli/flags.go, line 329 at r1 (raw file):

cli/start.go, line 117 at r1 (raw file):

sql/mon/mem_pool.go, line 35 at r1 (raw file):

sql/mon/mem_pool.go, line 51 at r1 (raw file):

sql/mon/mem_pool.go, line 52 at r1 (raw file):

sql/mon/mem_pool.go, line 52 at r1 (raw file):

sql/mon/mem_pool.go, line 56 at r1 (raw file):

sql/mon/mem_usage.go, line 70 at r1 (raw file):

sql/mon/mem_usage.go, line 73 at r1 (raw file):

sql/mon/mem_usage.go, line 76 at r1 (raw file):

sql/mon/mem_usage.go, line 95 at r1 (raw file):

sql/mon/mem_usage.go, line 164 at r1 (raw file):

sql/mon/mem_usage_mutex.go, line 24 at r1 (raw file):

sql/mon/mem_usage_mutex.go, line 26 at r1 (raw file):

Comments from Reviewable

Finished the entire first batch of comments, ready for the next one.

Review status: 0 of 23 files reviewed at latest revision, 24 unresolved discussions.

sql/mon/mem_usage.go, line 30 at r1 (raw file):

sql/mon/mem_usage.go, line 38 at r1 (raw file):

sql/mon/mem_usage.go, line 40 at r1 (raw file):

sql/mon/mem_usage.go, line 44 at r1 (raw file):

sql/mon/mem_usage.go, line 46 at r1 (raw file):

sql/mon/mem_usage.go, line 50 at r1 (raw file):

sql/mon/mem_usage.go, line 53 at r1 (raw file):

sql/mon/mem_usage.go, line 59 at r1 (raw file):

sql/mon/mem_usage.go, line 78 at r1 (raw file):

sql/mon/mem_usage.go, line 102 at r1 (raw file):

Comments from Reviewable

Tobi, you can't just close things that clutter your inbox.

Review status: 0 of 23 files reviewed at latest revision, 10 unresolved discussions, some commit checks failed.

sql/mon/mem_pool.go, line 51 at r1 (raw file):

sql/mon/mem_pool.go, line 31 at r2 (raw file):

// for memory.
// The API for this class is:
// - use a MemoryAccount per client to the pool;

nit: per client of the pool

sql/mon/mem_usage.go, line 44 at r1 (raw file):

sql/mon/mem_usage.go, line 50 at r1 (raw file):

sql/mon/mem_usage.go, line 53 at r1 (raw file):

And I'm still not big on the name preBudget. Still suggest ...reserve.

sql/mon/mem_usage.go, line 95 at r1 (raw file):

sql/mon/mem_usage.go, line 46 at r2 (raw file):

//   more instances of MemoryAccount, one per "category" of
//   allocation, and issue requests to Grow, Resize or Close to their
//   monitor.  Grow/Resize requests can be denied (return an error),

nit: double space (here and in the next paragraph). I've considered shutting up about these, and I probably should, but it hurts my eyes more than other stuff and I'm convinced that someone else will either complain now if they read this, or be compelled to fix later. But feel free to ignore :)

sql/mon/mem_usage.go, line 172 at r2 (raw file):

// Finally, a simplified API is provided in session_mem_usage.go to
// simplify the interface offered to SQL components using memory
// accounts linked to the session-bound monitor.

call out WrappedAccount by name?

sql/mon/mem_usage_mutex.go, line 26 at r1 (raw file):

Comments from Reviewable

Review status: 0 of 23 files reviewed at latest revision, 10 unresolved discussions.

sql/mon/mem_pool.go, line 51 at r1 (raw file):

sql/mon/mem_pool.go, line 31 at r2 (raw file):

sql/mon/mem_usage.go, line 44 at r1 (raw file):

sql/mon/mem_usage.go, line 50 at r1 (raw file):

sql/mon/mem_usage.go, line 53 at r1 (raw file):

sql/mon/mem_usage.go, line 95 at r1 (raw file):

sql/mon/mem_usage.go, line 46 at r2 (raw file):

sql/mon/mem_usage.go, line 172 at r2 (raw file):

sql/mon/mem_usage_mutex.go, line 26 at r1 (raw file):

Comments from Reviewable

I have performed the suggested refactorings, and some more. Included in this new revision:

• MemoryUsageMonitor renamed to MemoryMonitor
• MemoryPool goes away, MemoryMonitors can be stacked
• now tracking metrics separately for regular SQL clients, admin queries and internal executor
• viz in admin UI.

PTAL!

Note that I am rebasing this on top of #10094, so I will squash the additional 2 commits after #10094 is merged.

Review status: 0 of 39 files reviewed at latest revision, 20 unresolved discussions, all commit checks successful.

pkg/cli/flags.go, line 329 at r3 (raw file):

pkg/server/admin.go, line 48 at r3 (raw file):

pkg/sql/mem_metrics.go, line 26 at r3 (raw file):

pkg/sql/mem_metrics.go, line 27 at r3 (raw file):

pkg/sql/mem_metrics.go, line 80 at r3 (raw file):

pkg/sql/mon/mem_usage.go, line 59 at r3 (raw file):

pkg/sql/mon/mem_usage.go, line 69 at r3 (raw file):

pkg/sql/mon/mem_usage.go, line 204 at r3 (raw file):

1. regarding the order in which things happen in pgwire. I think it is a logic error to set up the Session object before the connection is up and authenticated (we don't want clients touching the executor before we know they're legit), so I changed that.

2. your comment highlighted a logic error that I have introduced in my refactoring, which I have now fixed. Thanks for bringing that up. Let me clarify: there is one top level shared monitor sqlMemoryPool. Then pgwire has one connMonitor attached to sqlMemoryPool; this takes care of "upfront reservations". Then separately there is one monitor per session, also attached to sqlMemoryPool (and thus has connMonitor as sibling, not parent). So whenever we think about ownership transfer here we have to be clear that this is transfer between siblings, not a (relatively simpler) transfer from parent to child.

3. I probably need to find a better name than "reserved". As long as I keep using the names "prebudget" or "reserved" you will keep thinking about accounts. But I still believe this is not the right way to think about this. To me, "reserved" is an amount dedicated to that monitor and not necessarily owned by another monitor. Remember, when I killed MemoryPool I brought the role of "root provider" into the responsibility of the monitor. For root monitors like sqlMemoryPool that reserved amount comes out of nothing. If there was an account for this, there would be no monitor to release it to, and worse, there would be no monitor to allocate it from.

If you find a good way to solve that last problem, in particular if you can tell me which invocation of Start() you would propose to use for sqlMemoryPool in pgwire/server.go, I'll be willing to look at this further.

pkg/sql/mon/mem_usage.go, line 238 at r3 (raw file):

pkg/sql/mon/mem_usage.go, line 468 at r3 (raw file):

sql/mon/mem_usage.go, line 333 at r2 (raw file):

Comments from Reviewable

Here are two screenshots of the UI additions:

• a new "SQL Memory" graph in the "System Resources" section per node:
  
• detailed graphs with histograms for the clients, admin and internal SQL endpoints in the "Advanced" section:
  

PTAL.
As discussed yesterday I have added a new BoundAccount to solve the ownership transfer of the reserved budget.
Also I have added one of the two acceptance tests we discussed. Please check this is going in the right direction? I still have to think some more about the other one.

nit: I don't think the "admin" and "internal" mem usage should be plotted on the "System Resources" section; just in the Advanced?

Looking good man, thanks for working with me. I just suggest a few improvements (IMHO) that I think would be worth-while.

Review status: 0 of 41 files reviewed at latest revision, 22 unresolved discussions, some commit checks failed.

pkg/cli/flags.go, line 329 at r3 (raw file):

pkg/cli/interactive_tests/test_sql_mem_monitor.tcl, line 1 at r7 (raw file):

#! /usr/bin/env expect -f

it's really nice that this works!

I was hoping we'd write this as an acceptance test. tcl is... exotic and creeping up in scope :)
In any case, the test deserves a comment about what it tests and what it'll do.

pkg/cli/interactive_tests/test_sql_mem_monitor.tcl, line 52 at r7 (raw file):

send "set database=information_schema;\r"
eexpect root@
send "select * from columns as a, columns as b, columns as c, columns as d limit 10;\r"

is this an outer join?

pkg/cli/interactive_tests/test_sql_mem_monitor.tcl, line 66 at r7 (raw file):

# Re-launch a server with very low limit for SQL memory
set spawn_id $shell_spawn_id
send "$argv start --max-sql-memory=30K\r"

ah, see, this is the crux of it, I think: I think part of the point of this test (or, rather, the point of all your work in general) has to be that the user should be able to set a limit that has something to do with the ulimit (ideally, exactly the ulimit), and our server is guaranteed to not crash. Like, it's "easy" to not crash if you refuse every query; the difficulty is in running close to your limit (so, having good utilization of the "hardware") and not crashing. Living on the edge.
I guess it's too early for that, since the rocksdb memory is not integrated with this your flag at all (although it does have its own flags), but at least aspirationally that's the dream, right? So maybe add a TODO here.

pkg/sql/mem_metrics.go, line 27 at r3 (raw file):

pkg/sql/row_container.go, line 71 at r7 (raw file):

// memory growth.
func (p *planner) NewRowContainer(
  sessionBound bool, h ResultColumns, rowCapacity int,

this new argument definitely needs some explanation :)

But... but... why not take a WrappedAccount, and let the caller worry about it.

In fact, it bothers me that the RowContainer has a reference to a planner. I don't see a good reason why something as dumb as a "container" has to know anything about a planner, or a session. Just give it an account on which it can perform all the operations it needs. Trade one pointer for another.

pkg/sql/row_container.go, line 105 at r7 (raw file):

  c.rows = nil
  c.varSizedColumns = nil
  var acc WrappedMemoryAccount

wait, what's going on here? We're making a WrappedAcc just so we can close it? Can't you use the correct monitor directly to close it? Or, better yet, as I'm saying above, I hope c.memAcc can be wrapped by a higher level.

pkg/sql/session.go, line 97 at r7 (raw file):

  // It is not directly used, but rather indirectly used via
  // sessionMon and txnMon. sessionMon tracks session-bound objects
  // like prepared statements and result sets. txnMon tracks txn-bound objects.

... like the running state of the planNodes in the midst of performing computation.

pkg/sql/session.go, line 99 at r7 (raw file):

  // like prepared statements and result sets. txnMon tracks txn-bound objects.
  mon        mon.MemoryMonitor
  sessionMon mon.MemoryMonitor

why were sessionMon and txnMon split? They don't seem to act as buffers. Is it just so that we get a chance to check for leaks after every transaction? If that is the case, maybe we actually want a per-statement monitor? Or maybe it has something to do with the metrics we want to export? In any case, I think it deserves a comment.

pkg/sql/session.go, line 100 at r7 (raw file):

  mon        mon.MemoryMonitor
  sessionMon mon.MemoryMonitor
  txnMon     mon.MemoryMonitor

should txnMon live in TxnState ?

pkg/sql/session.go, line 318 at r7 (raw file):

      s.memMetrics.TxnCurBytesCount,
      s.memMetrics.TxnMaxBytesHist,
      &s.mon, mon.BoundAccount{}, 1)

how come we're not using a larger increment? I think there might be some benefit to buffering in this txn-mon, even if the parent monitor is not contended.

pkg/sql/session_mem_usage.go, line 40 at r7 (raw file):

// WrappableMemoryAccount encapsulates a MemoryAccount to
// give it the W() method below.

the comment is stale, there's no W() any more.

But... but... what the hell is a WrappableMemoryAccount? :)
It seems to me that the W...() funs should be on the Session, and then we can get rid of this mind-bend.

pkg/sql/session_mem_usage.go, line 100 at r7 (raw file):

}

// StartMonitor interfaces between Session and mon.MemoryUsageMonitor

s/ MemUsageMon/MemMon

pkg/sql/session_mem_usage.go, line 105 at r7 (raw file):

      s.memMetrics.CurBytesCount,
      s.memMetrics.MaxBytesHist,
      pool, reserved, -1)

I think maybe a comment is warranted here... So we pass reserved to s.mon where it acts as a buffer, but we're not giving it to either sessionMon or txnMon. So those guys don't start with any buffer, so they'll need to ask their parents for memory immediately. But that's cool, cause the session is single threaded, and the point of buffering is avoiding contention.

pkg/sql/mon/bound_account.go, line 15 at r7 (raw file):

// permissions and limitations under the License.
//
// Author: Raphael 'kena' Poss (knz@cockroachlabs.com)

i'd put this in mem_usage.go, next to MemoryAccount, for discoverability.
Particularly since it adds a method to MemoryMonitor.

pkg/sql/mon/bound_account.go, line 39 at r7 (raw file):

func (mm *MemoryMonitor) MakeBoundAccount(
  ctx context.Context, initialAllocation int64,
) (res BoundAccount, err error) {

"we" generally dislike named ret vals, and here you're not using them other than as declarations

pkg/sql/mon/bound_account.go, line 49 at r7 (raw file):

// Close is an accessor for b.mon.CloseAccount.
func (b *BoundAccount) Close(ctx context.Context) {
  if b.mon != nil {

I think it's worth explaining why this can be nil - it's only nil for accounts used to initialize top-level monitors, which have basically gotten memory out of the ether.

... later - I just saw the MakeStandaloneBudget above. Maybe the comment belongs there, and a reference to that here :)

pkg/sql/mon/mem_usage.go, line 238 at r3 (raw file):

pkg/sql/mon/mem_usage.go, line 204 at r7 (raw file):

  // monitor. The reserved bytes are released to their owner monitor
  // upon Stop.
  reserved BoundAccount

ah, I feel at peace now. If I had a cigar, I'd put my feet on the table and smoke it.

pkg/sql/pgwire/server.go, line 116 at r7 (raw file):

      server.metrics.SQLMemMetrics.CurBytesCount,
      server.metrics.SQLMemMetrics.MaxBytesHist,
      nil, mon.MakeStandaloneBudget(maxSQLMem), 1)

0, or -1, instead of 1, to signal that the value is funky (it's not used by this pool, is it)?

Comments from Reviewable

Indeed I moved the detailed plot to "advanced".

Review status: 0 of 40 files reviewed at latest revision, 21 unresolved discussions.

pkg/cli/interactive_tests/test_sql_mem_monitor.tcl, line 1 at r7 (raw file):

pkg/cli/interactive_tests/test_sql_mem_monitor.tcl, line 52 at r7 (raw file):

pkg/cli/interactive_tests/test_sql_mem_monitor.tcl, line 66 at r7 (raw file):

pkg/sql/row_container.go, line 71 at r7 (raw file):

pkg/sql/row_container.go, line 105 at r7 (raw file):

pkg/sql/session.go, line 97 at r7 (raw file):

pkg/sql/session.go, line 99 at r7 (raw file):

For now the split is for a different reason, which is now documented in a comment.

pkg/sql/session.go, line 100 at r7 (raw file):

pkg/sql/session.go, line 318 at r7 (raw file):

pkg/sql/session_mem_usage.go, line 40 at r7 (raw file):

pkg/sql/session_mem_usage.go, line 100 at r7 (raw file):

pkg/sql/session_mem_usage.go, line 105 at r7 (raw file):

pkg/sql/mon/mem_usage.go, line 238 at r3 (raw file):

pkg/sql/mon/mem_usage.go, line 204 at r7 (raw file):

pkg/sql/pgwire/server.go, line 116 at r7 (raw file):

pkg/sql/mon/bound_account.go, line 15 at r7 (raw file):

pkg/sql/mon/bound_account.go, line 39 at r7 (raw file):

pkg/sql/mon/bound_account.go, line 49 at r7 (raw file):

Comments from Reviewable

🚢 it

Review status: 0 of 40 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

pkg/sql/row_container.go, line 71 at r7 (raw file):

pkg/sql/session.go, line 100 at r7 (raw file):

pkg/sql/session.go, line 96 at r9 (raw file):

  // mon tracks memory usage for SQL activity within this session. It
  // is not directly used, but rather indirectly used via sessionMon
  // and TxnState.mon. sessionMon tracks session-bound objects like prepared

there's no TxnState.mon. Although I still think there should be :)

pkg/sql/session_mem_usage.go, line 40 at r7 (raw file):

Comments from Reviewable

Review status: 0 of 40 files reviewed at latest revision, 7 unresolved discussions, some commit checks pending.

pkg/sql/row_container.go, line 71 at r7 (raw file):

pkg/sql/session.go, line 100 at r7 (raw file):

pkg/sql/session.go, line 96 at r9 (raw file):

pkg/sql/session_mem_usage.go, line 40 at r7 (raw file):

Comments from Reviewable