ui: update transaction insight details SQL query to not select from crdb_internal.ranges

[ericharmeling]

Fixes #94445.

This commit updates the SQL-over-HTTP query executed on behalf of the transaction insight details page to not select from the crdb_internal.ranges table, and instead select from other internal tables.

Loom: https://www.loom.com/share/c5f0ffc201ef44778ef724c8f53538fc

Release note: None

[cockroach-teamcity]

This change is 

[j82w]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 309 at r2 (raw file):

// txnContentionDetailsQuery selects information about a specific transaction contention event.
const txnContentionDetailsQuery = (id: string) => `

Why are we not just using the contention information from insights contention events?

cockroach/pkg/sql/crdb_internal.go

Line 6907 in 5a42279

contention_events JSONB,

[ericharmeling]

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

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 309 at r2 (raw file):

Previously, j82w (Jake) wrote…

Why are we not just using the contention information from insights contention events?

cockroach/pkg/sql/crdb_internal.go

Line 6907 in 5a42279

contention_events JSONB,

For a couple reasons:

1. This change works on both 22.2 and 23.1 and, if backported, it unblocks https://github.com/cockroachlabs/managed-service/pull/10854 on 22.2. Selecting from the contention_events column wouldn't work on 22.2.

2. IIUC, cluster_execution_insights captures statement executions whose execution time crosses a specific latency threshold. There could be a transaction with cumulative contention that surpasses the same threshold, but whose individual statements do not (and would therefore not be captured in cluster_execution_insights). But please correct me if I'm missing something.

[j82w]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling)

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 309 at r2 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

For a couple reasons:

1. This change works on both 22.2 and 23.1 and, if backported, it unblocks https://github.com/cockroachlabs/managed-service/pull/10854 on 22.2. Selecting from the contention_events column wouldn't work on 22.2.

2. IIUC, cluster_execution_insights captures statement executions whose execution time crosses a specific latency threshold. There could be a transaction with cumulative contention that surpasses the same threshold, but whose individual statements do not (and would therefore not be captured in cluster_execution_insights). But please correct me if I'm missing something.

1. That makes sense, but I'm a little surprised we would backport it 22.2 instead of just waiting for 23.1.
2. I thought the insights txn page was converted to pull all the information from cluster_execution_insights. That way it only shows query captured by the insights logic, and it's no longer a concern about the inconsistency in capture/retention policies. Since it from cluster_execution_insights it should already have all the contention events and not need to do an extra query.

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 334 at r2 (raw file):

               table_id
        FROM
          "".crdb_internal.tables] AS tables ON tce.contending_key BETWEEN crdb_internal.table_span(tables.table_id)[1]

What happens if the table index was dropped or changed? Should it still return the result but leave the names as "no longer exists".

[j82w]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling)

[ericharmeling]

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

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 309 at r2 (raw file):

I'm a little surprised we would backport it 22.2 instead of just waiting for 23.1.

@kevin-v-ngo IIUC, we do want insights on CC console for 22.2 serverless clusters, correct? If not, we might be able to simplify this query a bit using the newer 23.1 SHOW RANGES syntax. But I don't think we'll be able to join on cluster_execution_insights. contention_events for the reasons I mention below.

I thought the insights txn page was converted to pull all the information from cluster_execution_insights

The transaction insights page currently pulls from both cluster_execution_insights and transaction_contention_events.

The transactionInsights reducer uses the getTxnInsightEvents API function to pull from transaction_contention_events.

The executionInsights reducer uses getClusterInsightsApi to pull from cluster_execution_insights.

Then the selectTransactionInsights selector combines the transactionInsights and executionInsights slices. Entries with the same txn ID are merged, but if there are no duplicates (e.g., there is a contention event in transaction_contention_events but not in cluster_execution_insights), then the selector will just use the information in the slice with data to populate a row (e.g., a row with a txnID from transaction_contention_events that doesn't exist in cluster_execution_insights).

I think the end goal is to have all insights pages only pull from cluster_execution_insights, but that isn't what is happening right now.

---

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 334 at r2 (raw file):

Previously, j82w (Jake) wrote…

What happens if the table index was dropped or changed? Should it still return the result but leave the names as "no longer exists".

Good question! I don't know of a sensical way to determine if an index existed and now doesn't. Before, there was no index for a given contention event, we just defaulted to the primary index. But this query actually does select and return the primary index. I just changed this to "index not found" to cover this case.

[j82w]

pkg/ui/workspaces/cluster-ui/src/api/insightsApi.ts line 309 at r2 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

I'm a little surprised we would backport it 22.2 instead of just waiting for 23.1.

@kevin-v-ngo IIUC, we do want insights on CC console for 22.2 serverless clusters, correct? If not, we might be able to simplify this query a bit using the newer 23.1 SHOW RANGES syntax. But I don't think we'll be able to join on cluster_execution_insights. contention_events for the reasons I mention below.

I thought the insights txn page was converted to pull all the information from cluster_execution_insights

The transaction insights page currently pulls from both cluster_execution_insights and transaction_contention_events.

The transactionInsights reducer uses the getTxnInsightEvents API function to pull from transaction_contention_events.

The executionInsights reducer uses getClusterInsightsApi to pull from cluster_execution_insights.

Then the selectTransactionInsights selector combines the transactionInsights and executionInsights slices. Entries with the same txn ID are merged, but if there are no duplicates (e.g., there is a contention event in transaction_contention_events but not in cluster_execution_insights), then the selector will just use the information in the slice with data to populate a row (e.g., a row with a txnID from transaction_contention_events that doesn't exist in cluster_execution_insights).

I think the end goal is to have all insights pages only pull from cluster_execution_insights, but that isn't what is happening right now.

I'm actually changing my opinion to agree that it should just pull from the 'transaction_contention_events'. Duplicating the info in both insights and contention caches doesn't make sense. The 'transaction_contention_events' will very soon have all the waiting statement info too. LGTM

[ericharmeling]

bors r+ single on

[rail]

bors r-

[craig]

Canceled.

[rail]

bors r+ single off

[craig]

This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried.

Additional information:

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 40fcc96 to blathers/backport-release-22.2-94803: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.

---

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

[rail]

bors r-
We have temporarily disabled bors for merges. Please merge the PR manually when all required checks pass.