sql: make UI user active statements and open transactions consistent

[THardy98]

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.

[cockroach-teamcity]

This change is 

[jocrl]

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:

TestTrackOnlyExternalOpenTransactions

---

pkg/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]

Reviewed 1 of 7 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @jocrl, @maryliag, and @THardy98)

---

-- 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]

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

---

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)

[THardy98]

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

---

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.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

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 require library

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

[THardy98]

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

---

-- commits, line 11 at r2:

Previously, maryliag (Marylia Gutierrez) wrote…

this should have a release note, since you're changing the behaviour that users can notice

Done.

[Azhng]

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

---

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 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

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.

[THardy98]

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

---

pkg/ts/catalog/chart_catalog.go, line 2115 at r3 (raw file):

Previously, Azhng (Archer Zhang) wrote…

The AxisLabel looks 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 .. 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.

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.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"

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 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.

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 these row.Scan() go away.

Definitely useful, changes use SQLRunner, ty 😄

[maryliag]

Reviewed 2 of 7 files at r1, 2 of 5 files at r5, 1 of 2 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @jocrl, @maryliag, and @THardy98)

---

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 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.

Can you on the help of both metrics (active stmt and open queries) that they're filtering out internal?

[maryliag]

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

---

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...

[THardy98]

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

---

pkg/sql/exec_util.go, line 786 at r2 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

*can you add on...

In their tooltips?

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @jocrl, @kevin-v-ngo, and @maryliag)

---

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 ?

[THardy98]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @jocrl, @kevin-v-ngo, and @maryliag)

---

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.

[kevin-v-ngo]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @jocrl, @kevin-v-ngo, and @maryliag)

pkg/sql/exec_util.go, line 786 at r2 (raw file):

Previously, THardy98 (Thomas Hardy) 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 ?

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.

[jocrl]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @jocrl, @kevin-v-ngo, @maryliag, @stbof, and @THardy98)

---

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!

[maryliag]

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: complete! 1 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @stbof, and @THardy98)

[THardy98]

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: complete! 1 of 0 LGTMs obtained (waiting on @Annebirzin, @Azhng, @stbof, and @THardy98)

---

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"

[Azhng]

Nice!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Annebirzin, @stbof, and @THardy98)

[THardy98]

TFTR!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)