LGTM, left a couple questions.

But I do have a general thought:

Maybe this is because I've been working with the sql-over-http endpoint recently, but I wonder if we can use it to do the table/index id -> table/index name query, and do the remaining formatting logic UI side. We could potentially do the same for the prettify_statement query we use for the statement details page as well.

The main reason I suggest this is because I feel like we lose some information when we switch from id to name. A table name/index name combo could be the same for two different databases (which could be problematic when investigating), but a table id/index id combo will always be unique.

We also make this change for user-readability purposes which I feel could warrant changes more on the UI-side than DB-side. More broadly speaking, maybe we should be leveraging sql-over-http when possible to more cleanly separate the responsibilities of our UI/DB - handle our formatting logic UI-side and leave DB-side changes for gathering information (like the previous PR where you gather the used indexes of a query).

Would be interested in hearing your (or anyone else's) thoughts.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @kevin-v-ngo, and @maryliag)

pkg/server/combined_statement_stats.go line 861 at r1 (raw file):

			indexes = append(indexes, getIdxAndTableName(ctx, ie, idx))
		}
		metadata.Stats.Indexes = indexes

We're okay to replace table/index id with table/index name?

pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/plansTable.tsx line 196 at r1 (raw file):

  let indexInfo;
  for (let i = 0; i < indexes.length; i++) {
    if (indexes[i] === "dropped" || !indexes[i].includes("@")) {

Why do we increment the dropped count on the!indexes[i].includes("@") condition?

What would be an example of this scenario?

using sql-over-http

If I make the change to use it just for the conversion from id -> name, I would need its own cache, and we would need to make sure is in sync with the existing cache for the rest of the details, etc, which is not worth at this point. Since the goal is to eventually have all our endpoints using the sql-over-http, once we migrate the statement details, then we can have all in one place and will be easier to maintain. With the way I did, I don't even need to create a new parameter, I'm sending on the already existing stats and it's all in sync.

we lose some information when we switch from id to name
You need to remember this endpoint is being used for a statement fingerprint details, so it will be from one database. You won't see the same fingerprint using indexes from different databases. If you notice, when clicking on the table or index, it goes to its details page, and if you see the url has the database name on it, so it's going to that specific one. This means for this case we're not losing any information, everything we need is there for the user to see, and they also can check more details etc. Showing 112@1 instead of t1@t1_pkey would be very confusing.

So my approach is: this is just returning the values on the existing endpoint as is, have an issue to update the statement details endpoint to use sql-over-http.

Hopefully that helps with your questions :)

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @kevin-v-ngo, and @THardy98)

pkg/server/combined_statement_stats.go line 861 at r1 (raw file):

Yes, we're replacing to send to the UI only, not changing the values on the DB. We don't need the ids on the UI, just the name, but we save the IDs for the cases people rename them, so we need to get the latest value.

pkg/ui/workspaces/cluster-ui/src/statementDetails/planDetails/plansTable.tsx line 196 at r1 (raw file):

I added the check being overly protected, just in case something goes wrong with the values, but I agree that I should not be incrementing the dropped on this case. Made the changes to adjust the behaviour.

You need to remember this endpoint is being used for a statement fingerprint details, so it will be from one database.

Fair, I didn't consider that it's already scoped to a database.

If I make the change to use it just for the conversion from id -> name, I would need its own cache, and we would need to make sure is in sync with the existing cache for the rest of the details, etc, which is not worth at this point.

Curious why we would need a cache, and not fetch on page load (which I don't think would be prohibitively expensive) if we have the used indexes as part of the redux store?

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

that would be if we want to keep consistent with everything else that we keep on cache. This way we don't have to make additional calls in case we open the details, go back and open again.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dongniwang, @kevin-v-ngo, and @THardy98)

Looks good!