sql: Fix create partial stats test cases

[faizaanmadhani]

This commit modifies the create partial statistics test cases to ensure that the full table statistics that are to be used to create a partial statistic exist in the cache. Previously, this was not the case so certain tests had non-deterministic outputs causing failures in stress tests.

Resolves: #92495 and #92559

Epic: CRDB-19449

Release note: None

[cockroach-teamcity]

This change is 

[yuzefovich]

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

---

pkg/sql/logictest/testdata/logic_test/distsql_stats line 2080 at r1 (raw file):

# Test null values.
statement ok
SET CLUSTER SETTING sql.stats.automatic_collection.enabled = false;

nit: this should not be needed since we always disable auto stats in logic tests (see newCluster in logic.go) and in this file we don't enable them. What is the reason you included this? I'd guess that the bug you're fixing is due to table stats cache not being populated and not that some extra stats are being collected.

[faizaanmadhani]

pkg/sql/logictest/testdata/logic_test/distsql_stats line 2080 at r1 (raw file):

Previously, yuzefovich (Yahor Yuzefovich) wrote…

nit: this should not be needed since we always disable auto stats in logic tests (see newCluster in logic.go) and in this file we don't enable them. What is the reason you included this? I'd guess that the bug you're fixing is due to table stats cache not being populated and not that some extra stats are being collected.

I didn't realize we already disabled this, but this shouldn't be here anyways. Since this is disabled by default I've also removed it from when it's disabled above as well.

[michae2]

Nice, glad it worked!

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

---

pkg/sql/alter_table.go line 1379 at r1 (raw file):

	}

	if !settings.Version.IsActive(ctx, clusterversion.V23_1AddPartialStatisticsPredicateCol) {

If the JSONStatistics has a partial predicate, it would be wrong to insert it into table_statistics without the partial predicate, even if we haven't yet crossed the version gate. We need to throw an error if s.PartialPredicate != "".

---

pkg/sql/stats/new_stat.go line 106 at r1 (raw file):

					"nullCount",
					"avgSize",
					histogram

nit: was this already fixed in another PR?

---

pkg/sql/logictest/testdata/logic_test/distsql_stats line 2096 at r1 (raw file):

statement ok
ALTER TABLE a_null INJECT STATISTICS '$a_null_stats'

nit: Please leave comments at all of these ALTER TABLE INJECT STATISTICS here and below explaining that the purpose is to invalidate the stats cache.

[faizaanmadhani]

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

---

pkg/sql/stats/new_stat.go line 106 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

nit: was this already fixed in another PR?

It was supposed to be fixed in #92419, but that PR hasn't been merged in yet because there's a test there that keeps timing out. In order the keep the changes separate I can merge in that PR first and then merge in this one.

[faizaanmadhani]

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

---

pkg/sql/alter_table.go line 1379 at r1 (raw file):

Previously, michae2 (Michael Erickson) wrote…

If the JSONStatistics has a partial predicate, it would be wrong to insert it into table_statistics without the partial predicate, even if we haven't yet crossed the version gate. We need to throw an error if s.PartialPredicate != "".

Done.

[michae2]

Thank you!

Reviewed 1 of 3 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @faizaanmadhani, @mgartner, and @yuzefovich)

---

pkg/sql/alter_table.go line 1382 at r3 (raw file):

		if s.PartialPredicate != "" {
			return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, "statistic to insert is partial but cluster version is below 23.1")

nit: The injected statistics often contain dozens of objects, and can be several megabytes in size. Can we add something to this error message that helps identify which one was partial? Maybe the column name and the collection time?

---

pkg/sql/stats/new_stat.go line 106 at r1 (raw file):

Previously, faizaanmadhani (Faizaan Madhani) wrote…

It was supposed to be fixed in #92419, but that PR hasn't been merged in yet because there's a test there that keeps timing out. In order the keep the changes separate I can remove this change from this PR and we can merge in the other one first.

Sounds good!

[faizaanmadhani]

TFTRs! 🎉

bors r=michae2

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)