stats: fix missing autostats on cluster startup

[msirek]

Fixes #87247

Previously, automatic statistics may fail to be collected on tables
with no stats at cluster startup when the
sql.stats.automatic_collection.enabled cluster setting is false
but table storage parameter sql_stats_automatic_collection_enabled
is true.

Method ensureAllTables does not put entries into the settingOverrides
map, and since storage parameter info is not normally looked up in the
table header in auto stats processing, we fall back on the cluster
setting to determine if auto stats should be collected.

To address this, this patch modifies auto stats to flag whether the
current batch of tables saved in the mutationCounts map comes from
cluster startup processing, and if so, ensures that storage
parameters controlling auto stats are always looked up in the table
header during that processing.

Release note (bug fix): This patch fixes missing automatic statistics
collection at cluster startup when the
sql.stats.automatic_collection.enabled cluster setting is false,
but there are tables with storage parameter
sql_stats_automatic_collection_enabled set to true.

[cockroach-teamcity]

This change is 

[rytaft]

Thanks for the fix!

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @msirek)

---

pkg/sql/stats/automatic_stats.go line 249 at r1 (raw file):

	// refreshingAllTables is true on the initial stats collection pass at
	// cluster startup time.
	refreshingAllTables bool

Do we really need to add a new data member here? Looking at the changes you made below it feels like you could just use a local variable. I added a couple of comments where I think you could do that. Also doesn't seem like you really need to check mutationCounts.

---

pkg/sql/stats/automatic_stats.go line 400 at r1 (raw file):

			case <-initialTableCollection:
				r.ensureAllTables(ctx, &r.st.SV, initialTableCollectionDelay)

Couldn't you set a local variable to true here....

---

pkg/sql/stats/automatic_stats.go line 404 at r1 (raw file):

				mutationCounts := r.mutationCounts
				refreshingAllTables := r.refreshingAllTables
				r.refreshingAllTables = false

... and then copy it to a new variable and set the original local variable to false here?

---

pkg/sql/stats/automatic_stats_test.go line 596 at r1 (raw file):

		`CREATE DATABASE t;
		CREATE TABLE t.a (k INT PRIMARY KEY);
		ALTER TABLE t.a SET (sql_stats_automatic_collection_enabled = true);`)

Maybe add another table with the table setting false, and one with no table setting?

[msirek]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @michae2 and @rytaft)

---

pkg/sql/stats/automatic_stats.go line 249 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Do we really need to add a new data member here? Looking at the changes you made below it feels like you could just use a local variable. I added a couple of comments where I think you could do that. Also doesn't seem like you really need to check mutationCounts.

Modified it to use only local variables. As for checkingmutationCounts, this is done to avoid unnecessary table header lookups when the cluster setting is false and mutations occur in the first minute of the cluster being up.

---

pkg/sql/stats/automatic_stats.go line 400 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Couldn't you set a local variable to true here....

Done

---

pkg/sql/stats/automatic_stats.go line 404 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

... and then copy it to a new variable and set the original local variable to false here?

Done

---

pkg/sql/stats/automatic_stats_test.go line 596 at r1 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Maybe add another table with the table setting false, and one with no table setting?

Done

Code quote:

ALTER TABLE t.a SET (sql_stats_automatic_collection_enabled = true);`)

[rytaft]

Thanks!

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

[msirek]

TFTR!
bors r+

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

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)