sql: make UI user active statements and open transactions consistent#75815
sql: make UI user active statements and open transactions consistent#75815craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
|
This change is |
c3cf86c to
7d5c726
Compare
jocrl
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @jocrl and @THardy98)
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Measurement: "Active Statements", Unit: metric.Unit_COUNT, }
I'm just curious that your change also seems to be adding accounting for the number of active statements. Whereas the commit/PR message seems to imply that this was already being counted; just that you are changing the behavior?
Code quote:
MetaSQLActiveQueries = metric.Metadata{
Name: "sql.statements.active",
Help: "Number of SQL statements currently active",
Measurement: "Active Statements",
Unit: metric.Unit_COUNT,
}pkg/sql/conn_executor_test.go, line 1406 at r2 (raw file):
} func TestTrackOnlyExternalOpenTransactions(t *testing.T) {
Just curious; I don't know - is "external" the vocabulary for the opposite of "internal"? My cursory understanding was that the opposite of "internal" was "general user app stuff that IDK how to name".
Code quote:
TestTrackOnlyExternalOpenTransactionspkg/sql/conn_executor_test.go, line 1511 at r2 (raw file):
} func TestTrackOnlyExternalActiveStatements(t *testing.T) {
I'm wondering if you also want a test to make sure that the behavior of the two are consistent in that they both filter out internal? To guard against changes to one but not the other. WDYT?
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 1 of 7 files at r1.
Reviewable status:
-- commits, line 11 at r2:
this should have a release note, since you're changing the behaviour that users can notice
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
I'm just curious that your change also seems to be adding accounting for the number of active statements. Whereas the commit/PR message seems to imply that this was already being counted; just that you are changing the behavior?
+1
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_test.go, line 1430 at r2 (raw file):
waitingQuery, ) if err != nil {
The err should not be nil here. We can just remove the nil check.
pkg/sql/conn_executor_test.go, line 1444 at r2 (raw file):
) if err != nil { require.NoError(t, err, "executing %s ", selectQuery)
Having require.NoError can be problematic in testutils.SucceedsWithin block. This is because if the require check fails the the function won't be retried, the test runner will just fail the test immediately. Simply returns the error in this case, or you can annotate it with additional information. The SuceedsWithin will handle the test failure
pkg/sql/conn_executor_test.go, line 1493 at r2 (raw file):
if _, err := sqlDB.Exec("BEGIN"); err != nil { require.NoError(t, err, "executing %s ", "BEGIN") }
nit: simply do:
_, err := sqlDB.Exec("BEGIN')
require.NoError(t, err)Conditional err check is not necessary with require library
pkg/sql/conn_executor_test.go, line 1511 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
I'm wondering if you also want a test to make sure that the behavior of the two are consistent in that they both filter out internal? To guard against changes to one but not the other. WDYT?
Correct me if I'm wrong, I feel the verification logic can live in the same unit test instead of two? (Otherwise we are spinning up two test servers instead of one)
7d5c726 to
15a129e
Compare
THardy98
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
+1
Active statements was already being accounted for but included both internal & external active statements (sql.distsql.queries.active). The counting was being done at the SQL engine, which didn't provide enough context to determine whether the statement was internal/external. The introduced sql.statements.active counts active statements at a higher layer where internal/external context is available, and used to only counts external active statements.
pkg/sql/conn_executor_test.go, line 1406 at r2 (raw file):
Previously, jocrl (Josephine) wrote…
Just curious; I don't know - is "external" the vocabulary for the opposite of "internal"? My cursory understanding was that the opposite of "internal" was "general user app stuff that IDK how to name".
From my discussions with @Azhng, an "internal" transaction is any transaction executed by the InternalExecutor. Complementary, an external transaction is any transaction executed by any external executor (i.e. sql.DB.beginTxn(...)).
What's also worth knowing is that there are also "implicit" and "explicit" transactions. "Explicit" transactions occur when a user (explicitly) begins and ends a transaction with BEGIN and COMMIT. An "implicit" transaction is any single statement executed outside of an explicit transaction context (i.e. SELECT city from rides; is executed as a single statement, but becomes an "implicit" transaction under the hood, thereby carrying the same serializable guarantees a transaction provides).
@Azhng just want to make sure this is correct
pkg/sql/conn_executor_test.go, line 1430 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
The err should not be nil here. We can just remove the nil check.
Yup, nice catch. Done.
pkg/sql/conn_executor_test.go, line 1444 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Having
require.NoErrorcan be problematic intestutils.SucceedsWithinblock. This is because if therequirecheck fails the the function won't be retried, the test runner will just fail the test immediately. Simply returns the error in this case, or you can annotate it with additional information. TheSuceedsWithinwill handle the test failure
Oh didn't know that, nice. Removed the require.NoError checks inside of SucceedsWithin blocks.
pkg/sql/conn_executor_test.go, line 1493 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: simply do:
_, err := sqlDB.Exec("BEGIN') require.NoError(t, err)Conditional err check is not necessary with
requirelibrary
Removed, along with similar redundant error checks.
pkg/sql/conn_executor_test.go, line 1511 at r2 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Correct me if I'm wrong, I feel the verification logic can live in the same unit test instead of two? (Otherwise we are spinning up two test servers instead of one)
@Azhng Yes. I've combined these into the same unit test (especially since the test to check internal transactions and internal statements are effectively identical). Effectively just added the external statement case into the original open transaction test.
@jocrl I think simply having assertions to both transaction and statement counts after each statement execution should suffice. I considered adding the case where we execute an internal statement within an external transaction, but I think it would effectively be the same test as when we execute an internal statement outside of an external transaction. I don't think we can execute an external statement within an internal transaction, but could be wrong. Probably worth double checking my thoughts here @Azhng
8437212 to
ba04d7f
Compare
THardy98
left a comment
There was a problem hiding this comment.
Reviewable status:
Previously, maryliag (Marylia Gutierrez) wrote…
this should have a release note, since you're changing the behaviour that users can notice
Done.
Azhng
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ts/catalog/chart_catalog.go, line 2115 at r3 (raw file):
"sql.statements.active.internal", }, AxisLabel: "Transactions",
The AxisLabel looks off
pkg/sql/conn_executor_test.go, line 1406 at r2 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
From my discussions with @Azhng, an "internal" transaction is any transaction executed by the
InternalExecutor. Complementary, an external transaction is any transaction executed by any external executor (i.e.sql.DB.beginTxn(...)).What's also worth knowing is that there are also "implicit" and "explicit" transactions. "Explicit" transactions occur when a user (explicitly) begins and ends a transaction with
BEGINandCOMMIT. An "implicit" transaction is any single statement executed outside of an explicit transaction context (i.e.SELECT city from rides;is executed as a single statement, but becomes an "implicit" transaction under the hood, thereby carrying the same serializable guarantees a transaction provides).@Azhng just want to make sure this is correct
Yep, your understanding is correct. Basically we can summarize it into a 2x2 table.
| External Txn (or just regular Txns issued by the user) | Internal Txn | |
|---|---|---|
| Implicit Txn | Using regular connExecutor without BEGIN/COMMIT |
Using InternalExecutor, where txn param is nil |
| Explicit Txn | Using regular connExecutor with BEGIN/COMMIT |
Using InternalExecutor, where txn param is not nil |
pkg/sql/conn_executor_test.go, line 1511 at r2 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
@Azhng Yes. I've combined these into the same unit test (especially since the test to check internal transactions and internal statements are effectively identical). Effectively just added the external statement case into the original open transaction test.
@jocrl I think simply having assertions to both transaction and statement counts after each statement execution should suffice. I considered adding the case where we execute an internal statement within an external transaction, but I think it would effectively be the same test as when we execute an internal statement outside of an external transaction. I don't think we can execute an external statement within an internal transaction, but could be wrong. Probably worth double checking my thoughts here @Azhng
Yes. A transaction is bound to an executor. So if a transaction is an external transaction, I don't think we have any codepath that would execute an internal query in the context of the external transaction. And the mere thoughts of executing user-supplied query in an internal executor is scaring me, since we often override the security settings when we use internal executor.
pkg/sql/conn_executor_test.go, line 1417 at r3 (raw file):
sqlServer := s.SQLServer().(*sql.Server) waitingQuery := `SELECT pg_sleep(60)` selectQueryInternal := `SELECT query_id FROM crdb_internal.cluster_queries WHERE query = '` + waitingQuery + `'`
minor nit: selectQueryIDInternal
pkg/sql/conn_executor_test.go, line 1418 at r3 (raw file):
waitingQuery := `SELECT pg_sleep(60)` selectQueryInternal := `SELECT query_id FROM crdb_internal.cluster_queries WHERE query = '` + waitingQuery + `'` selectQueryExternal := `SELECT query_id FROM [SHOW CLUSTER STATEMENTS] WHERE query = '` + waitingQuery + `'`
minor nit, up to you if you wanna do this: there are couple ways you can do this SHOW .. STATEMENTS query,
for querying the external statements, you can do [SHOW STATEMENTS] and for both internal and external statements you can do SHOW ALL STATEMENTS.
pkg/sql/conn_executor_test.go, line 1458 at r3 (raw file):
require.Equal(t, int64(0), sqlServer.Metrics.EngineMetrics.SQLActiveStatements.Value()) _, err := sqlServer.GetExecutorConfig().InternalExecutor.ExecEx(
Hmm I wonder if the cancellation query can be issued using a regular SQL Connection. It might makes the code here a bit less verbose?
pkg/sql/conn_executor_test.go, line 1465 at r3 (raw file):
cancelQueryInternal, ) require.NoError(t, err, "executing %s ", cancelQueryInternal)
The third param in require.NoError is the error message the test runner prints out if the validation condition fails. It usually goes in the form of expect .... to happen, but <something else unexpected happened> . So here, it would be something like "expect the waiting query to be successfully cancelled, but it was not"
pkg/sql/conn_executor_test.go, line 1473 at r3 (raw file):
nil, sessiondata.InternalExecutorOverride{User: security.RootUserName()}, selectQueryInternal,
Hmm seems like this is to test whether the previous pg_sleep() has been properly cancelled yet. I feel this doesn't need to be wrapped in a SucceedsWithin since we know for sure at this point the query must be cancelled. Also I feel there's probably no need to use the internal executor here.
pkg/sql/conn_executor_test.go, line 1536 at r3 (raw file):
require.NoError(t, err, "executing %s ", cancelQueryExternal) testutils.SucceedsWithin(t, func() error {
same here, I feel the retry loop is probably not necessary.
pkg/sql/conn_executor_test.go, line 1545 at r3 (raw file):
if err != gosql.ErrNoRows { return errors.New("pg_sleep has not been cancelled yet, still active") }
Btw have you tried to use sqlutil.MakeSQLRunner()? They have a couple really neat interfaces (e.g. QueryStr()) that can make a lot of these row.Scan() go away.
e2715a1 to
7b62261
Compare
THardy98
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/ts/catalog/chart_catalog.go, line 2115 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
The
AxisLabellooks off
Whoops, fixed.
pkg/sql/conn_executor_test.go, line 1417 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
minor nit:
selectQueryIDInternal
The query now uses count(*) to see if the query is active (i.e. exists in crdb_internal.cluster_queries or SHOW STATEMENTS).
Renamed to selectInternalQueryActive/selectExternalQueryActive.
pkg/sql/conn_executor_test.go, line 1418 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
minor nit, up to you if you wanna do this: there are couple ways you can do this
SHOW .. STATEMENTSquery,for querying the external statements, you can do
[SHOW STATEMENTS]and for both internal and external statements you can doSHOW ALL STATEMENTS.
Changed to use [SHOW STATEMENTS] instead of [SHOW CLUSTER STATEMENTS].
pkg/sql/conn_executor_test.go, line 1458 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm I wonder if the cancellation query can be issued using a regular SQL Connection. It might makes the code here a bit less verbose?
No longer need cancellation queries now that we're using contention to hang queries instead of pg_sleep().
pkg/sql/conn_executor_test.go, line 1465 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
The third param in
require.NoErroris the error message the test runner prints out if the validation condition fails. It usually goes in the form ofexpect .... to happen, but <something else unexpected happened>. So here, it would be something like "expect the waiting query to be successfully cancelled, but it was not"
Changed to use the expect... but format for require.NoError.
pkg/sql/conn_executor_test.go, line 1473 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Hmm seems like this is to test whether the previous
pg_sleep()has been properly cancelled yet. I feel this doesn't need to be wrapped in aSucceedsWithinsince we know for sure at this point the query must be cancelled. Also I feel there's probably no need to use the internal executor here.
Now use contention to make a query hang instead of pg_sleep(), no longer cancel queries.
pkg/sql/conn_executor_test.go, line 1536 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
same here, I feel the retry loop is probably not necessary.
With the contention logic, we now check that the queries are no longer hanging once we COMMIT the transaction and the contention is resolved.
pkg/sql/conn_executor_test.go, line 1545 at r3 (raw file):
Previously, Azhng (Archer Zhang) wrote…
Btw have you tried to use
sqlutil.MakeSQLRunner()? They have a couple really neat interfaces (e.g.QueryStr()) that can make a lot of theserow.Scan()go away.
Definitely useful, changes use SQLRunner, ty 😄
22d69a6 to
e4f9ee6
Compare
maryliag
left a comment
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, 2 of 5 files at r5, 1 of 2 files at r6.
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
Active statements was already being accounted for but included both internal & external active statements (
sql.distsql.queries.active). The counting was being done at the SQL engine, which didn't provide enough context to determine whether the statement was internal/external. The introducedsql.statements.activecounts active statements at a higher layer where internal/external context is available, and used to only counts external active statements.
Can you on the help of both metrics (active stmt and open queries) that they're filtering out internal?
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
Can you on the help of both metrics (active stmt and open queries) that they're filtering out internal?
*can you add on...
e4f9ee6 to
30391cf
Compare
THardy98
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
*can you add on...
In their tooltips?
maryliag
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, THardy98 (Thomas Hardy) wrote…
In their tooltips?
yes, so something like
Help: "Number of non-internal SQL statements currently active",
not sure if we decided on usage of non-internal vs external. Any opinios @kevin-v-ngo or @Annebirzin ?
30391cf to
8305cb0
Compare
THardy98
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, maryliag (Marylia Gutierrez) wrote…
yes, so something like
Help: "Number of non-internal SQL statements currently active",not sure if we decided on usage of non-internal vs external. Any opinios @kevin-v-ngo or @Annebirzin ?
Clarified that we are keeping track of non-internal active statements/open transactions in the metrics metadata Help.
We can say, "The total number of running SQL statements across all nodes." Adding @stbof for any other suggestions External seems odd ('user' would be better here) and i don't see us using the term anywhere: https://www.cockroachlabs.com/docs/stable/ui-statements-page.html#filter FYI - no other charts quality internal vs user so i'd keep it consistent for now. |
aa4f65f to
79d45ef
Compare
jocrl
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/sql/conn_executor_test.go, line 1511 at r2 (raw file):
I think simply having assertions to both transaction and statement counts after each statement execution should suffice.
Ah makes sense, thanks!
9d5bd5f to
0aa6224
Compare
maryliag
left a comment
There was a problem hiding this comment.
Great!
btw, can you update the title on the PR from external to user to align with our decision of naming convention?
Reviewed 4 of 4 files at r12, all commit messages.
Reviewable status:
There was a problem hiding this comment.
Updated the PR title from external to user. Will do so for the commit message as well (which unfortunately triggers another CI run...).
Reviewable status:
pkg/sql/exec_util.go, line 786 at r2 (raw file):
Previously, kevin-v-ngo wrote…
We can say, "The total number of running SQL statements across all nodes." Adding @stbof for any other suggestions
External seems odd ('user' would be better here) and i don't see us using the term anywhere: https://www.cockroachlabs.com/docs/stable/ui-statements-page.html#filter
FYI - no other charts quality internal vs user so i'd keep it consistent for now.
Changed to "user" instead of "external" and "non-internal"
Previously, the number of Active SQL Statements could be greater than the number of Open SQL Transactions on the UI. This inconsistency was due to filtering out internal Open SQL Transactions but keeping internal Active SQL Statements. This change fixes this inconsistency by filtering out both internal Active SQL Statements and internal Open SQL Transactions. Release note (sql change): filter out internal statements and transactions from UI timeseries metrics.
0aa6224 to
406139e
Compare
Azhng
left a comment
There was a problem hiding this comment.
Nice!
Reviewable status:
|
TFTR! bors r+ |
|
Build succeeded: |
Previously, the number of Active SQL Statements could be greater than
the number of Open SQL Transactions on the UI. This inconsistency was
due to filtering out internal Open SQL Transactions but keeping
internal Active SQL Statements. This change fixes this inconsistency by
filtering out both internal Active SQL Statements and internal Open SQL
Transactions.
Resolves: #70228
Release note (sql change): filter out internal statements and
transactions from UI timeseries metrics.