sql: add columns to node_execution_insights #85131

j82w · 2022-07-27T14:30:20Z

This commit adds the following new columns
to crdb_internal.node_execution_insights table.

txn_id                     UUID NOT NULL,
txn_fingerprint_id         BYTES NOT NULL,
query                      STRING NOT NULL,
status                     STRING NOT NULL,
start_time                 TIMESTAMP NOT NULL,
end_time                   TIMESTAMP NOT NULL,
full_scan                  BOOL NOT NULL,
user_name                  STRING NOT NULL,
application_name           STRING NOT NULL,
database_name              STRING NOT NULL,
plan_gist                  STRING NOT NULL,
rows_read                  INT8 NOT NULL,
rows_written               INT8 NOT NULL,
priority                   FLOAT NOT NULL,
retries                    INT8 NOT NULL

Part of #81024

Release note (sql change): Adding txn_id,
txn_fingerprint_id, query, status, start_time,
end_time, full_scan, user_name, application_name,
database_name, plan_gist, rows_read, rows_written,
priority, and retries columns to
crdb_internal.node_execution_insights

cockroach-teamcity · 2022-07-27T14:30:27Z

This change is

matthewtodd

🎉 Woo! Thanks, Jake! Left a few comments inline.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @matthewtodd)

pkg/sql/executor_statement_metrics.go line 190 at r1 (raw file):

		StatementError:       stmtErr,
		IndexRecommendations: planner.instrumentation.indexRecommendations,
		Query:                stmt.StmtNoConstants,

I think "no constants" is a good choice for now. In #81023 we'll want to have the option to show the constants to users with permission. (The constants, though sensitive, can be helpful when understanding why the execution may have been slow.) cc @kevin-v-ngo

pkg/sql/sqlstats/ssprovider.go line 18 at r1 (raw file):

import (
	"context"
	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"

You can dev build crlfmt and have Goland use it to keep these imports organized. (I think the linter will complain if you don't?)

pkg/sql/sqlstats/insights/insights.proto line 42 at r1 (raw file):

    (gogoproto.casttype) = "github.com/cockroachdb/cockroach/pkg/roachpb.StmtFingerprintID"];
  double latency_in_seconds = 3;
  string transaction_id = 4;

Remove this one.

pkg/sql/sqlstats/insights/insights.proto line 45 at r1 (raw file):

  string query = 5;
  string status = 6;
  google.protobuf.Timestamp start_time = 7[(gogoproto.nullable) = false, (gogoproto.stdtime) = true];

nit: add a space between the 7 and the [.

pkg/sql/sqlstats/insights/insights.proto line 53 at r1 (raw file):

  string plan_gist = 13;
  int64 rowsRead = 14;
  int64 rowsWritten = 15;

nit: rows_read and rows_written. (Snake case for protobuf field names.)

pkg/sql/sqlstats/ssmemstorage/ss_mem_writer.go line 45 at r1 (raw file):

var _ sqlstats.Writer = &Container{}

func GetStatusString(statementError error) string {

nit: getStatusString. (To make it private.)

pkg/sql/sqlstats/insights/integration/insights_test.go line 68 at r1 (raw file):

	require.NoError(t, err)

	// Execute a fast statement that is under the latencyThreshold to verify it does not get logged

We have other, more focused tests that do this. See detector_test.go. This broader integration test is here mostly to show something working end-to-end; the branching out details are tested in faster unit tests lower in the stack.

pkg/sql/sqlstats/insights/integration/insights_test.go line 84 at r1 (raw file):

	}, 1*time.Second)

	// Verify time

nit: I think our style guide requires that comments be full, punctuated sentences.

pkg/sql/sqlstats/insights/integration/insights_test.go line 92 at r1 (raw file):

			"end_time, "+
			"full_scan "+
			"FROM crdb_internal.node_execution_insights")

I feel ambivalent about testing the specifics of the data up here in this integration test, but I don't have any better ideas at the moment. :-)

kevin-v-ngo · 2022-07-27T17:21:18Z

@j82w and @maryliag - do you think we can integrate the CREATE index recommendations in this table as well? #83782 (comment)

Having Create index recommendations for 'outliers' and slow executions would really complete the e2e experience with being actionable for suboptimal plans.

maryliag

@kevin-v-ngo once that feature is added, it can be added to this table. Right now it would be empty.

Reviewed 3 of 8 files at r1, 3 of 3 files at r2.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w, @maryliag, and @matthewtodd)

-- commits line 24 at r2:
nit: the release notes is used for the docs team, so try being more descriptive to help them out, listing the column name (you don't need to specify, just the names are enough)

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

	session_id                 STRING NOT NULL,
	txn_id                     UUID NOT NULL,
  txn_fingerprint_id         BYTES NOT NULL,

this line seems to be misaligned (space vs tabs perhaps?)

pkg/sql/sqlstats/insights/integration/insights_test.go line 109 at r2 (raw file):

	testutils.SucceedsWithin(t, func() error {
		rows, err := conn.QueryContext(ctx, "SELECT "+
			"crdb_internal.decode_plan_gist(plan_gist) "+

FYI, it is possible to have null values for plan_gist, so you would need to check if the value is not null, before calling this function.
But I don't think this test is necessary here, because it's testing the decode function, but we already have tests for that.

kevin-v-ngo · 2022-07-27T17:45:56Z

@kevin-v-ngo once that feature is added, it can be added to this table. Right now it would be empty.

I guess my question is more on when will it be available? And if possible, can we have it as part of the V1 experience here.

matthewtodd · 2022-07-27T18:00:48Z

@maryliag @j82w regarding release notes, I've been a little lax on these PRs (since a lot was in flux as we were figuring things out), instead aiming to lay out a first cohesive overview in the eventual PR that flips the cluster settings on by default.

maryliag

when will it be available? And if possible, can we have it as part of the V1 experience here
it will be available soon (by beginning of August), hopefully we can add to the V1

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

j82w

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

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

Previously, maryliag (Marylia Gutierrez) wrote…

this line seems to be misaligned (space vs tabs perhaps?)

Fixed, it was using spaces instead of tabs.

pkg/sql/sqlstats/ssprovider.go line 18 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

You can dev build crlfmt and have Goland use it to keep these imports organized. (I think the linter will complain if you don't?)

Done.

pkg/sql/sqlstats/insights/insights.proto line 42 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Remove this one.

Done.

pkg/sql/sqlstats/insights/integration/insights_test.go line 68 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

We have other, more focused tests that do this. See detector_test.go. This broader integration test is here mostly to show something working end-to-end; the branching out details are tested in faster unit tests lower in the stack.

Done.

pkg/sql/sqlstats/insights/integration/insights_test.go line 92 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

I feel ambivalent about testing the specifics of the data up here in this integration test, but I don't have any better ideas at the moment. :-)

If you don't verify the data in the integration test how can you be sure that the data did not get corrupted and/or incorrectly converted at some point?

matthewtodd

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @j82w, @maryliag, and @matthewtodd)

pkg/sql/sqlstats/insights/integration/insights_test.go line 92 at r1 (raw file):

Previously, j82w wrote…

If you don't verify the data in the integration test how can you be sure that the data did not get corrupted and/or incorrectly converted at some point?

I think I figured out why I was feeling that -- I had written this test first as a design tool, to help me see when I had gotten the right bits connected end-to-end, where showing something working (a row in the table!) gave me the confidence I needed at the time.

But I think you're probably on to something here: it may make sense for this test to now shift into more of a regression coverage tool, where we use it to make sure nothing gets broken going forward. And in that context, checking the data is certainly useful.

maryliag

Reviewed 1 of 2 files at r3, 2 of 2 files at r4, 1 of 3 files at r5, 3 of 3 files at r6, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w)

j82w · 2022-07-28T16:20:28Z

bors r+

kevin-v-ngo · 2022-07-28T16:25:32Z

pkg/sql/crdb_internal.go

+	query                      STRING NOT NULL,
+	status                     STRING NOT NULL,
+	start_time                 TIMESTAMP NOT NULL,
+	end_time                   TIMESTAMP NOT NULL,


When are we going to derive elapsed time?

kevin-v-ngo · 2022-07-28T16:25:51Z

pkg/sql/crdb_internal.go

+	end_time                   TIMESTAMP NOT NULL,
+	full_scan                  BOOL NOT NULL,
+	user_name                  STRING NOT NULL,
+	application_name           STRING NOT NULL,


kevin-v-ngo · 2022-07-28T16:30:08Z

pkg/sql/crdb_internal.go

+	session_id                 STRING NOT NULL,
+	txn_id                     UUID NOT NULL,
+	txn_fingerprint_id         BYTES NOT NULL,
+	statement_id               STRING NOT NULL,


Let's abbreviate statement -> stmt

statement_id -> stmt_id
stmt_fingerprint_id -> stmt_fingerprint_id

Seems like that's the pattern: https://www.cockroachlabs.com/docs/stable/crdb-internal.html#statement_statistics

kevin-v-ngo · 2022-07-28T16:43:29Z

pkg/sql/crdb_internal.go

@@ -6233,20 +6233,60 @@ func populateClusterLocksWithFilter(
 var crdbInternalNodeExecutionInsightsTable = virtualSchemaTable{
 	schema: `
 CREATE TABLE crdb_internal.node_execution_insights (


From the brief, seems like we're just missing the following:

retryable
last_retry_reason
last_contended_row
index_recommendation

j82w · 2022-07-28T16:53:16Z

bors r-

craig · 2022-07-28T16:53:18Z

Canceled.

j82w · 2022-07-28T17:46:24Z

bors r+

j82w

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @j82w and @kevin-v-ngo)

pkg/sql/crdb_internal.go line 6235 at r6 (raw file):

Previously, kevin-v-ngo wrote…

From the brief, seems like we're just missing the following:

retryable
last_retry_reason
last_contended_row
index_recommendation

This PR was marked as partial change for the issue. Those columns are more complicated to implement. To avoid making the PR to large I'm going to do a separate follow up PR. I will fix the column names specified in the other comments at that time too.

pkg/sql/crdb_internal.go line 6239 at r6 (raw file):

Previously, kevin-v-ngo wrote…

Let's abbreviate statement -> stmt

statement_id -> stmt_id
stmt_fingerprint_id -> stmt_fingerprint_id

Seems like that's the pattern: https://www.cockroachlabs.com/docs/stable/crdb-internal.html#statement_statistics

This will be fixed in a follow up PR. Merging this now to unblock other tasks.

pkg/sql/crdb_internal.go line 6244 at r6 (raw file):

Previously, kevin-v-ngo wrote…

When are we going to derive elapsed time?

Elapsed time can easily be computed when needed.

pkg/sql/crdb_internal.go line 6247 at r6 (raw file):

Previously, kevin-v-ngo wrote…

app_name

This will be fixed in a follow up PR. Merging this now to unblock other tasks.

craig · 2022-07-28T19:53:53Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-07-29T04:48:22Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-07-29T07:52:02Z

Build failed:

GitHub CI (Cockroach)

This commit adds the following new columns to crdb_internal.node_execution_insights table. txn_id UUID NOT NULL, txn_fingerprint_id BYTES NOT NULL, stmt_id STRING NOT NULL, stmt_fingerprint_id BYTES NOT NULL, query STRING NOT NULL, status STRING NOT NULL, start_time TIMESTAMP NOT NULL, end_time TIMESTAMP NOT NULL, full_scan BOOL NOT NULL, user_name STRING NOT NULL, app_name STRING NOT NULL, database_name STRING NOT NULL, plan_gist STRING NOT NULL, rows_read INT8 NOT NULL, rows_written INT8 NOT NULL, priority FLOAT NOT NULL, retries INT8 NOT NULL Part of #81024 Release note (sql change): Renaming statement to stmt, and transaction to txn in columns. Adding txn_fingerprint_id, query, status, start_time, end_time, full_scan, user_name, app_name, database_name, plan_gist, rows_read, rows_written, priority, and retries columns to crdb_internal.node_execution_insights

matthewtodd · 2022-07-29T15:44:29Z

Since we've got a green build and I'm eager to ship my branch that depends on these changes, I'm going to go ahead and kick off the bors process again:

bors r+

craig · 2022-07-29T16:03:00Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-07-29T16:38:18Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2022-07-29T18:55:51Z

Build succeeded:

GitHub CI (Cockroach)

j82w requested a review from a team July 27, 2022 14:47

j82w marked this pull request as ready for review July 27, 2022 14:47

j82w requested a review from a team July 27, 2022 14:47

j82w requested a review from a team as a code owner July 27, 2022 14:47

matthewtodd suggested changes Jul 27, 2022

View reviewed changes

maryliag reviewed Jul 27, 2022

View reviewed changes

j82w commented Jul 27, 2022

View reviewed changes

matthewtodd approved these changes Jul 27, 2022

View reviewed changes

maryliag approved these changes Jul 28, 2022

View reviewed changes

kevin-v-ngo requested changes Jul 28, 2022

View reviewed changes

kevin-v-ngo reviewed Jul 28, 2022

View reviewed changes

j82w commented Jul 28, 2022

View reviewed changes

craig bot merged commit 933df73 into cockroachdb:master Jul 29, 2022

cockroach-teamcity mentioned this pull request Jul 29, 2022

sql: add columns to node_execution_insights cockroachdb/docs#14669

Closed

j82w mentioned this pull request Aug 25, 2022

ui: fix cluster_execution_insights priority value #86900

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: add columns to node_execution_insights #85131

sql: add columns to node_execution_insights #85131

j82w commented Jul 27, 2022 •

edited

Loading

cockroach-teamcity commented Jul 27, 2022

matthewtodd left a comment

kevin-v-ngo commented Jul 27, 2022

maryliag left a comment

kevin-v-ngo commented Jul 27, 2022

matthewtodd commented Jul 27, 2022

maryliag left a comment

j82w left a comment

matthewtodd left a comment

maryliag left a comment

j82w commented Jul 28, 2022

kevin-v-ngo Jul 28, 2022

kevin-v-ngo Jul 28, 2022

kevin-v-ngo Jul 28, 2022

kevin-v-ngo Jul 28, 2022

j82w commented Jul 28, 2022

craig bot commented Jul 28, 2022

j82w commented Jul 28, 2022

j82w left a comment

craig bot commented Jul 28, 2022

craig bot commented Jul 29, 2022

craig bot commented Jul 29, 2022

matthewtodd commented Jul 29, 2022

craig bot commented Jul 29, 2022

craig bot commented Jul 29, 2022

craig bot commented Jul 29, 2022

sql: add columns to node_execution_insights #85131

sql: add columns to node_execution_insights #85131

Conversation

j82w commented Jul 27, 2022 • edited Loading

cockroach-teamcity commented Jul 27, 2022

matthewtodd left a comment

Choose a reason for hiding this comment

kevin-v-ngo commented Jul 27, 2022

maryliag left a comment

Choose a reason for hiding this comment

kevin-v-ngo commented Jul 27, 2022

matthewtodd commented Jul 27, 2022

maryliag left a comment

Choose a reason for hiding this comment

j82w left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

j82w commented Jul 28, 2022

kevin-v-ngo Jul 28, 2022

Choose a reason for hiding this comment

kevin-v-ngo Jul 28, 2022

Choose a reason for hiding this comment

kevin-v-ngo Jul 28, 2022

Choose a reason for hiding this comment

kevin-v-ngo Jul 28, 2022

Choose a reason for hiding this comment

j82w commented Jul 28, 2022

craig bot commented Jul 28, 2022

j82w commented Jul 28, 2022

j82w left a comment

Choose a reason for hiding this comment

craig bot commented Jul 28, 2022

craig bot commented Jul 29, 2022

craig bot commented Jul 29, 2022

matthewtodd commented Jul 29, 2022

craig bot commented Jul 29, 2022

craig bot commented Jul 29, 2022

craig bot commented Jul 29, 2022

j82w commented Jul 27, 2022 •

edited

Loading