sql: add new index_recommendation column

[maryliag]

This commit adds a new column index_recommendations
STRING[] to:
crdb_internal.node_statement_statistics
crdb_internal.cluster_statement_statistics
system.statement_statistics
crdb_internal.statement_statistics

Part of #83782

Release note (sql change): Adding new column index_recommendations
to crdb_internal.node_statement_statistics,
crdb_internal.cluster_statement_statistics, system.statement_statistics
and crdb_internal.statement_statistics

[cockroach-teamcity]

This change is 

[maryliag]

Since I keep rebasing and after a few days get another conflict on cockroach_version and key_string files, I'll wait to get approval on this PR and do the final merge

friendly ping @cockroachdb/sql-schema (for schema change), @cockroachdb/sql-queries (for the changes here) and @cockroachdb/sql-observability

[postamar]

Indeed, at this time in the release cycle, avoiding these kinds of merge conflicts is a bit of a lost cause. Just a couple of minor comments on Schema's behalf.

Reviewed 1 of 25 files at r1, 1 of 9 files at r2, 7 of 13 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag and @postamar)

---

pkg/sql/catalog/systemschema/system.go line 484 at r3 (raw file):

    ) STORED,

    index_recommendations STRING[],

Consider making this NOT NULL DEFAULT (array[]::STRING[])) instead? This way you avoid having to worry about NULL values altogether, which is always annoying with array types. You wouldn't need to add that function in tree either.

---

pkg/upgrade/upgrades/alter_statement_statistics_index_recommendations.go line 31 at r3 (raw file):

func alterSystemStatementStatisticsAddIndexRecommendations(
	ctx context.Context, cs clusterversion.ClusterVersion, d upgrade.TenantDeps, _ *jobs.Job,
) error {

It's worth adding a test for this upgrade step, just like the other upgrades. When spinning up your test cluster at version clusterversion.AlterSystemStatementStatisticsAddIndexRecommendations, do an ALTER TABLE ... DROP COLUMN to drop the index_recommendations column from the bootstrapped table to pretend that the table was created when bootstrapping from an earlier binary. Otherwise you're just testing the idempotency of the addIndexRecommendationsColstatement.

edit: me not use reviewable gud, test is there and lgtm

[mgartner]

LGTM on Queries's (Query's?) behalf.

Reviewed 12 of 25 files at r1, 4 of 9 files at r2, 13 of 13 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

[postamar]

I found a few minor inconsistencies, not sure how they relate to the current test failures. Otherwise I have nothing to complain about on Schema's behalf.

Reviewed 17 of 17 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @maryliag)

---

pkg/sql/catalog/systemschema/system.go line 2015 at r4 (raw file):

					Hidden:      true,
				},
				{Name: "index_recommendations", ID: 12, Type: types.StringArray, Nullable: false},

Doesn't this also need some default expr field set now?

---

pkg/upgrade/upgrades/alter_statement_statistics_index_recommendations.go line 25 at r4 (raw file):

const addIndexRecommendationsCol = `
ALTER TABLE system.statement_statistics
ADD COLUMN IF NOT EXISTS "index_recommendations" STRING[] DEFAULT (array[]::STRING[]))

Isn't NOT NULL missing here also?

I'm dismissing my own review

[maryliag]

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

---

pkg/sql/catalog/systemschema/system.go line 484 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

Consider making this NOT NULL DEFAULT (array[]::STRING[])) instead? This way you avoid having to worry about NULL values altogether, which is always annoying with array types. You wouldn't need to add that function in tree either.

Done

---

pkg/sql/catalog/systemschema/system.go line 2015 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Doesn't this also need some default expr field set now?

Done

---

pkg/upgrade/upgrades/alter_statement_statistics_index_recommendations.go line 31 at r3 (raw file):

Previously, postamar (Marius Posta) wrote…

It's worth adding a test for this upgrade step, just like the other upgrades. When spinning up your test cluster at version clusterversion.AlterSystemStatementStatisticsAddIndexRecommendations, do an ALTER TABLE ... DROP COLUMN to drop the index_recommendations column from the bootstrapped table to pretend that the table was created when bootstrapping from an earlier binary. Otherwise you're just testing the idempotency of the addIndexRecommendationsColstatement.

Talked offline, the test was already added

---

pkg/upgrade/upgrades/alter_statement_statistics_index_recommendations.go line 25 at r4 (raw file):

Previously, postamar (Marius Posta) wrote…

Isn't NOT NULL missing here also?

Done

[THardy98]

Left a small comment, not familiar with the schema changes in pkg/upgrade, but I imagine other 👀 have taken a look.

[maryliag]

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

---

pkg/sql/sqlstats/persistedsqlstats/flush.go line 479 at r5 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…

Should we also check for conflict on index recommendations here as well?
Is there a case where we want to insert into an existing stats row if our index recommendations have changed?

the conflict is for the things that shouldn't change from execution to execution, and the index recommendations (like plan, statistics) can change. So you try to insert, if the "key" values already exists, it will returns there was a conflict, so then it will use the update statement, that will update the recommendation column

[THardy98]

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

pkg/sql/sqlstats/persistedsqlstats/flush.go line 479 at r5 (raw file):

Previously, THardy98 (Thomas Hardy) wrote…
the conflict is for the things that shouldn't change from execution to execution, and the index recommendations (like plan, statistics) can change. So you try to insert, if the "key" values already exists, it will returns there was a conflict, so then it will use the update statement, that will update the recommendation column

Ahh okay, makes sense 👍

[THardy98]

LGTM 👍

[maryliag]

TFTR!!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)