Skip to content

sql: show allocated memory in SHOW SESSIONS#25395

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
jordanlewis:show-session
Jun 4, 2018
Merged

sql: show allocated memory in SHOW SESSIONS#25395
craig[bot] merged 1 commit intocockroachdb:masterfrom
jordanlewis:show-session

Conversation

@jordanlewis
Copy link
Copy Markdown
Member

@jordanlewis jordanlewis commented May 10, 2018

Release note (sql change): SHOW SESSIONS now includes the number of
allocated bytes by the session.

@jordanlewis jordanlewis requested review from a team, asubiotto and knz May 10, 2018 04:04
@cockroach-teamcity
Copy link
Copy Markdown
Member

cockroach-teamcity commented May 10, 2018

This change is Reviewable

@asubiotto
Copy link
Copy Markdown
Contributor

asubiotto commented May 19, 2018

Sorry for the delay in reviewing this!

The objective of this change is to offer operators the ability to observe memory usage per session at the current point in time but I don't think this change is enough to allow for this because according to my understanding

  1. The monitor whose usage is reported does not take into account any distsql usage. The distsql monitor is hooked up directly to the root monitor, so bypassing this session monitor. That should be fixed for distsql execution local to the node where the session was created, although seems right for remote distsql execution on behalf of a session.
  2. After fixing 1), this approach would only report memory usage of the session's gateway node so we would want some way to annotate remote monitors with a session ID and aggregate the usage.

I think this would be great to have, and some extensions could include showing the high watermark memory usage of a query (by using the same mechanism with statement monitors).

@jordanlewis jordanlewis requested a review from a team May 25, 2018 21:48
@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented May 25, 2018

Okay, PTAL. I added the high watermark of memory usage to the session. Also, I rebased this patch on top of #26108, which makes the query's txn memory monitor parent the flow memory monitor (for the gateway node only).

@rjnn
Copy link
Copy Markdown
Contributor

rjnn commented May 29, 2018

:lgtm:

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.

pkg/sql/crdb_internal.go, line 846 at r2 (raw file):

			oldestStartDatum,
			kvTxnIDDatum,
			tree.NewDInt(tree.DInt(session.AllocBytes)),

This should default to zero in a mixed version cluster where it gets session information from a machine that has the old version, correct?

Comments from Reviewable

@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented May 29, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

pkg/sql/crdb_internal.go, line 846 at r2 (raw file):

Previously, arjunravinarayan (Arjun Narayan) wrote…

This should default to zero in a mixed version cluster where it gets session information from a machine that has the old version, correct?

Correct.

Comments from Reviewable

@asubiotto
Copy link
Copy Markdown
Contributor

asubiotto commented May 29, 2018

:lgtm: Although maybe you should call out in the release note that memory usage of a session's remote execution is not included.

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

Comments from Reviewable

@jordanlewis jordanlewis force-pushed the show-session branch 2 times, most recently from 4ac24d0 to 281def5 Compare June 4, 2018 16:11
@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Jun 4, 2018

Although maybe you should call out in the release note that memory usage of a session's remote execution is not included.

Done.

bors r+

@jordanlewis jordanlewis requested a review from a team June 4, 2018 16:23
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Canceled

@jordanlewis
sql: show allocated memory in SHOW SESSIONS
5e12498 
```
root@localhost:26257/defaultdb> select session_id, active_queries, alloc_bytes, max_alloc_bytes from [show sessions];
+----------------------------------+--------------------------------------------------------------------------+-------------+-----------------+
|            session_id            |                              active_queries                              | alloc_bytes | max_alloc_bytes |
+----------------------------------+--------------------------------------------------------------------------+-------------+-----------------+
| 153200ed085836810000000000000001 | SELECT session_id, active_queries, alloc_bytes, max_alloc_bytes FROM     |           0 |           30720 |
|                                  | [SHOW CLUSTER SESSIONS]                                                  |             |                 |
| 153201699798a3170000000000000001 | SELECT * FROM lineitem JOIN a ON l_orderkey = a                          |      256000 |         1914880 |
+----------------------------------+--------------------------------------------------------------------------+-------------+-----------------+
```

Release note (sql change): `SHOW SESSIONS` now includes the number of
currently allocated bytes by the session, and the maximum number of
allocated bytes that the session ever owned at once. n.b. these numbers
don't include the bytes allocated for this session by remote nodes.
@jordanlewis jordanlewis requested a review from a team as a code owner June 4, 2018 21:03
@jordanlewis
Copy link
Copy Markdown
Member Author

jordanlewis commented Jun 4, 2018

bors r+

craig bot pushed a commit that referenced this pull request Jun 4, 2018
@jordanlewis
Merge #25395 #25860
44264c9 
25395: sql: show allocated memory in `SHOW SESSIONS` r=jordanlewis a=jordanlewis

Release note (sql change): `SHOW SESSIONS` now includes the number of
allocated bytes by the session.

25860: distsql: support tuples r=arjunravinarayan a=arjunravinarayan

Release note (feature): using tuples in a query no longer reverts you
to single node local SQL execution.

Co-authored-by: Jordan Lewis <jordanthelewis@gmail.com>
Co-authored-by: Arjun Narayan <arjun@cockroachlabs.com>
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Jun 4, 2018

Build succeeded

@craig craig bot merged commit 5e12498 into cockroachdb:master Jun 4, 2018
@jordanlewis jordanlewis deleted the show-session branch June 5, 2018 16:51
@jordanlewis jordanlewis mentioned this pull request Jun 5, 2018
Closed
@jseldess jseldess mentioned this pull request Jul 2, 2018
Closed
@asubiotto asubiotto mentioned this pull request Sep 14, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@knz knz Awaiting requested review from knz

@asubiotto asubiotto Awaiting requested review from asubiotto

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jordanlewis @cockroach-teamcity @asubiotto @rjnn