sql,server: change indexusagestats subsystem to issue cluster RPC fanout

[Azhng]

sql: introduce crdb_internal.index_usage_stats virtual table

This commit introduce crdb_internal.index_usage_stats virtual
table that is backed by new clusterindexusagestats package. This
new package implements a variant of the indexusagestats interface
and serves the data by issuing cluster RPC fanout.

Addresses #64740

Followup to #66451

Release note (sql change): introduce crdb_internal.index_usage_statistics
virtual table to surface index usage statistics.
sql.metrics.index_usage_stats.enabled cluster setting can be used to
turn on/off the subsystem. It is default to true.

[cockroach-teamcity]

This change is 

[blathers-crl]

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

• We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
• When CI has completed, please ensure no errors have appeared.

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

[maryliag]

your "follow up"on the PR description is pointing to itself :)

Reviewed 2 of 21 files at r3, 31 of 31 files at r4.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @mgartner)

---

pkg/server/index_usage_stats_test.go, line 215 at r4 (raw file):

	// When being stressed, there's possibility that it might takes a while for
	// the stats payload to be ingested.
	// time.Sleep(3 * time.Second)

nit: remove unnecessary comments

---

pkg/server/index_usage_stats_test.go, line 218 at r4 (raw file):

	// Check local node stats.
	// Fetch stats reader from each individual

nit: period

---

pkg/sql/idxusage/cluster_idx_usage_stats.go, line 18 at r4 (raw file):

)

// ClusterIndexUsageStats implements the Reader interface and is able to returns

nit: return

[Azhng]

Fixed!

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

[Azhng]

Friendly ping @mgartner

[mgartner]

Reviewed 2 of 21 files at r3, 9 of 31 files at r4, 4 of 9 files at r5, 4 of 5 files at r6.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @maryliag)

---

pkg/server/index_usage_stats.go, line 55 at r6 (raw file):

			statsReader :=
				s.sqlServer.pgServer.SQLServer.GetLocalIndexStatisticsReader()
			return serializeIndexUsageStats(req, statsReader)

Should req be localReq here?

---

pkg/server/index_usage_stats.go, line 58 at r6 (raw file):

		}

		statusServer, err := s.dialNode(ctx, requestedNodeID)

nit: maybe name statusServer to statusClient or client to make it clear its not of type statusServer and that it's a client to a remote status server

---

pkg/server/index_usage_stats.go, line 63 at r6 (raw file):

		}

		return statusServer.IndexUsageStatistics(ctx, localReq)

Should localReq be req here?

---

pkg/server/index_usage_stats.go, line 87 at r6 (raw file):

	var combinedError error
	errFn := func(_ roachpb.NodeID, nodeFnError error) {
		if nodeFnError != nil {

nit: iterateNodes should not call this function with a nil error, so you can remove this check.

---

pkg/server/index_usage_stats.go, line 115 at r6 (raw file):

	}

	err := reader.ForEach(idxusage.IteratorOptions{

Creating a reader interface and implementing a local and cluster implementation of the interface only for the purpose of using either here seems over-engineered for what needs to be accomplished: aggregating index usage stats response from multiple nodes. Also, the cluster implementation just uses a local implementation which has all the complexity and overhead of dealing with concurrent reads and write. Would it be simpler to remove the interface, implement a single struct for local index usage stats, and perform aggregation of the results here?

---

pkg/server/index_usage_stats.go, line 118 at r6 (raw file):

		SortedTableID: req.OrderedTableID,
		SortedIndexID: req.OrderedIndexID,
		Max:           max,

It'd be nice to not fetch more than max usage stats from remote nodes in the first place, if possible. Or at least to fetch only up to max from each node because we'll never need more than that. If that's a lot more effort, maybe leave a TODO somewhere?

---

pkg/server/serverpb/status.proto, line 1126 at r6 (raw file):

  // by the RPC. If this field is left unset, then all stats data will be
  // returned at once.
  oneof max {

Did you try using optional here instead of oneof?

---

pkg/sql/idxusage/cluster_idx_usage_stats.go, line 26 at r6 (raw file):

	// usage stats, therefore, we do not call LocalIndexUsageStats.Start() method.
	storage      *LocalIndexUsageStats
	statusServer serverpb.SQLStatusServer

nit: this field is never used

[Azhng]

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

---

pkg/server/index_usage_stats.go, line 87 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: iterateNodes should not call this function with a nil error, so you can remove this check.

done.

---

pkg/server/index_usage_stats.go, line 115 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Creating a reader interface and implementing a local and cluster implementation of the interface only for the purpose of using either here seems over-engineered for what needs to be accomplished: aggregating index usage stats response from multiple nodes. Also, the cluster implementation just uses a local implementation which has all the complexity and overhead of dealing with concurrent reads and write. Would it be simpler to remove the interface, implement a single struct for local index usage stats, and perform aggregation of the results here?

I agree this is a bit awkward. I think the root cause here is that this RPC endpoint is consumed by two different clients:

1. The code that populates crdb_internal virtual table. This client needs an aggregated reader so it can populate the virtual table.
2. The DB Console. This client needs the serialized data returning from the RPC endpoint. Preferably aggregated. If not, the DB Console will aggregated in the frontend.

The max param was introduced here to distinguish between those two type of use case. But this does introduce quite a bit of awkwardness.

I think a better approach would be: this endpoint does not perform aggregation at all. It returns all the statistics directly to the caller without aggregation. Then the code responsible for crdb_internal virtual table would be the one performing aggregation.

Then, the DB Console will be calling to a different endpoint, (possibly the v2 status API that's being built right now) and that API will be responsible to fetching data from the crdb_internal virtual table.

I updated the code to reflect this.

---

pkg/server/index_usage_stats.go, line 118 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

It'd be nice to not fetch more than max usage stats from remote nodes in the first place, if possible. Or at least to fetch only up to max from each node because we'll never need more than that. If that's a lot more effort, maybe leave a TODO somewhere?

Removed max from the request protobuf now since this API will be primary used for populating the crdb_internal virtual table as mentioned in above comment. There's not quite necessary to limit number of rows returning from this endpoint.

---

pkg/server/serverpb/status.proto, line 1126 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Did you try using optional here instead of oneof?

Removed max param here. See the comment above.

---

pkg/sql/idxusage/cluster_idx_usage_stats.go, line 26 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

nit: this field is never used

Done.

[mgartner]

Reviewed 15 of 16 files at r7.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @mgartner)

---

pkg/server/index_usage_stats.go, line 63 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Should localReq be req here?

Looking at this again, and I think localReq is correct. This is basically telling another node "give me your local stats", is that correct? Might be worth a comment for clarity.

---

pkg/server/index_usage_stats.go, line 115 at r6 (raw file):

Previously, Azhng (Archer Zhang) wrote…

I agree this is a bit awkward. I think the root cause here is that this RPC endpoint is consumed by two different clients:

1. The code that populates crdb_internal virtual table. This client needs an aggregated reader so it can populate the virtual table.
2. The DB Console. This client needs the serialized data returning from the RPC endpoint. Preferably aggregated. If not, the DB Console will aggregated in the frontend.

The max param was introduced here to distinguish between those two type of use case. But this does introduce quite a bit of awkwardness.

I think a better approach would be: this endpoint does not perform aggregation at all. It returns all the statistics directly to the caller without aggregation. Then the code responsible for crdb_internal virtual table would be the one performing aggregation.

Then, the DB Console will be calling to a different endpoint, (possibly the v2 status API that's being built right now) and that API will be responsible to fetching data from the crdb_internal virtual table.

I updated the code to reflect this.

Sounds simpler to me. 👍

---

pkg/server/index_usage_stats_test.go, line 195 at r7 (raw file):

	}()

	// Records a non-full scan.

nit add: "over t_a_idx".

---

pkg/server/index_usage_stats_test.go, line 199 at r7 (raw file):

	require.NoError(t, err)

	// Records an zigzag join.

nit add: "that scans both t_a_idx and t_b_idx"

---

pkg/server/index_usage_stats_test.go, line 203 at r7 (raw file):

	require.NoError(t, err)

	// Record a full scan.

nit add: "over t_b_idx"

---

pkg/server/index_usage_stats_test.go, line 213 at r7 (raw file):

	// When being stressed, there's possibility that it might takes a while for
	// the stats payload to be ingested.
	// time.Sleep(3 * time.Second)

Is this leftover?

---

pkg/server/index_usage_stats_test.go, line 260 at r7 (raw file):

		statsEntries++
		switch stats.Key.IndexID {
		case 2: // t@t_a_idx

nit: could this instead be case indexKeyA.IndexID?

---

pkg/sql/conn_executor.go, line 272 at r7 (raw file):

	pool *mon.BytesMonitor

	// indexUsageStats tracks the index usage statistics.

nit: Make it clear in this comment that it only tracks usage for the gateway node of a query, not the entire cluster.

[Azhng]

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

---

pkg/server/index_usage_stats.go, line 63 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Looking at this again, and I think localReq is correct. This is basically telling another node "give me your local stats", is that correct? Might be worth a comment for clarity.

Yes. I'll add a comment.

---

pkg/server/index_usage_stats.go, line 115 at r6 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Sounds simpler to me. 👍

Done.

---

pkg/server/index_usage_stats_test.go, line 213 at r7 (raw file):

Previously, mgartner (Marcus Gartner) wrote…

Is this leftover?

Ah yes. I cleaned this up earlier but probably messed up the rebase and it sneaked back. 🤦

[mgartner]

Reviewed 1 of 16 files at r7, 3 of 3 files at r8.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @maryliag and @mgartner)

---

pkg/server/index_usage_stats.go, line 63 at r6 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Yes. I'll add a comment.

Thanks!

[Azhng]

TFTR!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)