can you add tests to verify the new values being returned?

Reviewed 13 of 16 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @gtr)

-- commits, line 11 at r1:
nit: include

pkg/sql/closed_session_cache.go, line 31 at r1 (raw file):

	"sql.closed_session_cache.capacity",
	"the maximum number of sessions in the cache",
	1000, // TODO(gtr): Totally arbitrary for now, adjust later.

have you done any testing to adjust this number?

pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.fixture.ts, line 50 at r1 (raw file):

    max_alloc_bytes: Long.fromNumber(10240),
    active_queries: [],
    status: Status.OPEN,

can you add some examples with status closed?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/server/status.go, line 202 at r1 (raw file):

	registry := b.sessionRegistry
	sessions := registry.SerializeAll()
	closedSessions := b.closedSessionCache.GetSerializedSessions()

Just want to point out a subtle concurrency issue here.

// the mutex on the registry is dropped after function returns,
// suppose we have a sessionA in `sessions` since sessionA is active.
sessions := registry.SerializeAll()

/*
 * sessionA finishes. Since mutex on the active session registry is dropped, sessionA moves from
 * the active registry into the closed session cache.
 */

// closedSessions now contains sessionA. The API response now will contain two entries of
// sessionA showing that sessionA is both "active" and "closed" at the same time.
closedSessions := b.closedSessionCache.GetSerializedSessions()

You might want to do some manual mutex management here to prevent that from happening.

pkg/sql/conn_executor.go, line 1779 at r1 (raw file):

	ex.planner.extendedEvalCtx.setSessionID(ex.sessionID)
	defer ex.server.cfg.SessionRegistry.deregister(ex.sessionID, ex.queryCancelKey)
	defer ex.server.cfg.ClosedSessionCache.add(ex.sessionID, ex.serialize())

optional style nit: i'd just put two function into a single defer block

defer func() {
  ex.server.cfg.SessionRegistry.deregistry()
  ex.server.cfg.ClosedSessionCache.add()
}()

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/server/status.go, line 202 at r1 (raw file):

Good catch! I locked the session registry while we get the serialized sessions from the closed session cache.

pkg/sql/conn_executor.go, line 1779 at r1 (raw file):

Done.

I dont see any memory accounting for the new structure. Where can I find it?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

Thanks for the heads up! I embedded a*mon.BytesMonitor object inside ClosedSessionCache.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.fixture.ts, line 50 at r1 (raw file):

I think the scope of the PR is getting a bit long; should these tests be added when i get to the UI part? These were added simply to get rid of compilation errors for the newly added status field in serverpb.Session. Also note the new IDLE status, which is defined in pkg/sql/conn_executor.go:

status := serverpb.Session_IDLE
if len(activeQueries) > 0 {
    status = serverpb.Session_OPEN
}

Reviewed 1 of 11 files at r2, 1 of 12 files at r3, 17 of 22 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/server/status.go, line 2757 at r5 (raw file):

		}
		sessions := nodeResp.([]serverpb.Session)
		sort.Slice(sessions, func(i, j int) bool {

nit: you don't need to do this sort since you are sorting again after appending the sessions

pkg/ui/workspaces/cluster-ui/src/sessions/sessionsPage.fixture.ts, line 50 at r1 (raw file):

Sure, they can be added during the UI PR. But I would like to see on the PR description some examples of the responses that include open, idle and closed sessions.

pkg/server/status_test.go, line 3377 at r5 (raw file):

		}

		require.Equal(t, 5, open)

Avoid using the same value for 2 different things, because they might be returned switched values and you wouldn't know. Make one value for open and another for closed

Reviewed 1 of 16 files at r1, 4 of 22 files at r5.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/server/status.go, line 202 at r1 (raw file):

Hmm, you might want to lock the sessionRegistry before calling .SerializeAll() (this will result in deadlock, but you can create a new function called .SerializedAllLocked().

The code currently still have unprotected critical section (between .SerializeAll() and the .Lock()) which can result in race condition.

pkg/sql/closed_session_cache.go, line 176 at r5 (raw file):

	size += 16            // size of ClusterWideID
	size += n.data.Size() // size of serverpb.Session
	size += 8             // size of time.Time

You sure time.Time only takes 8 bytes? Seems like it has 2 * 64 bit integer + a pointer type.

type Time struct {
	wall uint64
	ext  int64

	loc *Location
}

A trick here would be

unsafe.Sizeof(time.Time{})

pkg/server/status_test.go, line 3265 at r5 (raw file):

		ReplicationMode: base.ReplicationManual,
		ServerArgs: base.TestServerArgs{
			Insecure: true,

Why are we spinning up an insecure cluster here?

pkg/server/status_test.go, line 3274 at r5 (raw file):

	doSessionsRequest := func(client http.Client, username string) listSessionsResponse {
		req, err := http.NewRequest("GET", ts1.AdminURL()+"/_status/sessions", nil)

we have serverutils.GetJSONProto for all this boilerplates

pkg/server/status_test.go, line 3302 at r5 (raw file):

			RawQuery: "sslmode=disable",
		}
		db, err := gosql.Open("postgres", pgURL.String())

have you tried serverutils.OpenDBConnE :P

pkg/server/status_test.go, line 3311 at r5 (raw file):

	// Create three users.
	users := [3]string{"test_user_a", "test_user_b", "test_user_c"}
	conn, err := testCluster.ServerConn(0).Conn(ctx)

Can we just use testCluster.ServerConn(0) here? iirc .Conn(ctx) may or may not create a new connection.

pkg/server/status_test.go, line 3324 at r5 (raw file):

	if err != nil {
		t.Fatal(err)
	}

small nit: require.NoError

pkg/server/status_test.go, line 3349 at r5 (raw file):

			targetDB := getUserConn(t, user, testCluster.Server(0))
			defer targetDB.Close()
			sqlutils.MakeSQLRunner(targetDB).Exec(t, `SELECT version()`)

Did you have to execute something for the session to be consider active?

pkg/server/status_test.go, line 3353 at r5 (raw file):

	}

	time.Sleep(500 * time.Millisecond)

Hmm why the half a second sleep here?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/server/status.go, line 202 at r1 (raw file):

Good catch! I added a method SerializedAllLocked() to the session registry and kept the session registry locked until we serialize all the sessions.

Code quote:

		// Before retrieving all the closed sessions, prevent any changes to the
		// session registry by locking the session registry. Without this, a session

pkg/server/status.go, line 2757 at r5 (raw file):

Done!

Code quote:

sort.Slice(sessions, func(i, j int) bool {

pkg/server/status_test.go, line 3265 at r5 (raw file):

I based this test off TestListSessionsV2 which uses an insecure connection but no reason other than that. I removed the insecure setting and added passwords for each user.

pkg/server/status_test.go, line 3274 at r5 (raw file):

Thanks! Simplifies the code a lot :)

pkg/server/status_test.go, line 3302 at r5 (raw file):

I might be wrong on this but it looks like serverutils.OpenDBConnE opens a gosql DB connection using the root user:

pgURL, cleanupGoDB, err := sqlutils.PGUrlE(
		sqlAddr, "StartServer" /* prefix */, url.User(security.RootUser))

but I want to open a connection using a specific username and password combination.

Code quote:

db, err := gosql.Open("postgres", pgURL.String())

pkg/server/status_test.go, line 3311 at r5 (raw file):

Good catch, done!

pkg/server/status_test.go, line 3324 at r5 (raw file):

Done.

pkg/server/status_test.go, line 3349 at r5 (raw file):

If you mean "active" as in a session is started, yes. But this question made me think of the IDLE status. So I also decided to test if we get the correct response for IDLE vs OPEN sessions by running SELECT pg_sleep(5) to keep a session open. (see PR description).

pkg/server/status_test.go, line 3353 at r5 (raw file):

No reason, deleted!

Code quote:

time.Sleep(500 * time.Millisecond)

pkg/server/status_test.go, line 3377 at r5 (raw file):

Got it! Done.

Nice! Almost there!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/sql/closed_session_cache.go, line 54 at r12 (raw file):

	mu struct {
		syncutil.RWMutex
		mon  *mon.BytesMonitor

nit: you can move the bytes monitor out the mu struct since it has its own mutex to protect it.

For bound accounts, you can optionally assign it an mutex to give the bound account threadsafety.

pkg/sql/closed_session_cache.go, line 91 at r12 (raw file):

	node := &sessionNode{id: id, data: session, timestamp: c.timeSrc()}

	acc := c.mu.mon.MakeBoundAccount()

Hmm I should've caught this earlier, we don't want to make a new bound account each time .add() is called. What we want is to initialize a bound account once when we initialize ClosedSessionCache, and close it once when we are destroying the this object. (however, in this case, ClosedSessionCache has the same object lifetime as the SQL Server, so it's really getting destroyed only when the SQL Server shuts down.

The consequence of initializing bound account inside .add() is that, because of the defer acc.Close(ctx) call, the memory account becomes no-op, which defeat the purpose the memory accounting.

pkg/sql/closed_session_cache.go, line 174 at r12 (raw file):

func (n *sessionNode) size() int {
	return int(unsafe.Sizeof(time.Time{}))

Ah my bad, what I meant was to replace the hardcoded 8 byte with this line. What I had in mind was

size := 0
size += int(unsafe.Sizeof(ClusterWideID{}))
size += n.data.Size()
size += int(unsafe.Sizeof(time.Time{}))
return size

pkg/server/status_test.go, line 3377 at r5 (raw file):

Hmm I don't see the SucceedSoon wraps? accidentally reverted ?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/sql/closed_session_cache.go, line 54 at r12 (raw file):

Done.

pkg/sql/closed_session_cache.go, line 91 at r12 (raw file):

Good catch, I initialized the bound account when we initialize the ClosedSessionCache and call c.mu.acc.Grow() in the .add() method.

pkg/sql/closed_session_cache.go, line 174 at r12 (raw file):

My bad, fixed!

pkg/server/status_test.go, line 3377 at r5 (raw file):

Yup, added it back.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/sql/closed_session_cache.go, line 72 at r13 (raw file):

		ShouldEvict: func(size int, _, _ interface{}) bool {
			capacity := ClosedSessionCacheCapacity.Get(&st.SV)
			return int64(size) > capacity

Remember to shrink the bound account when you evicts the older data. Otherwise we would just be leaking memory here. There should be a callback here you can register.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/sql/closed_session_cache.go, line 72 at r13 (raw file):

Good catch, I used the OnEvictedEntry callback function.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/sql/closed_session_cache.go, line 174 at r12 (raw file):

Ugh, doesn't seem like it's fixed 😅

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

pkg/sql/closed_session_cache.go, line 174 at r12 (raw file):

size := 0
size += int(unsafe.Sizeof(ClusterWideID{}))
size += n.data.Size()
size += int(unsafe.Sizeof(time.Time{}))
return size

Ok got it this time haha

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @gtr, and @maryliag)

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng, @gtr, @maryliag, and @xinhaoz)

pkg/sql/crdb_internal.go, line 1898 at r16 (raw file):

Yes, they are for clearer distinction. Right now the tables only list open sessions but I plan on also listing closed sessions; I'm saving that for a future PR.