sqlstats: add limit to flush data

[maryliag]

Previously, we would always let data get flushed to system tables, then the cleanup job would remove the excess data.
When the cleanup job have hiccups and get stuck data was still continuously being added, making the situation worst and requiring reset of stats in some cases.

To prevent this cases, this commit is adding a limit of how much excess data can be flushed, this way if the cleanup job stops working, the data won't blow up.

Part Of #97074

Follow up work can do a better job at cleaning the data during the flush, but this commit focus on adding the safety so it can be backported.

Release note (sql change): Add a hard limit of how much data can be flushed to system tables for sql stats.

[cockroach-teamcity]

This change is 

[j82w]

-- commits line 13 at r1:
What is the concern with the data blowing up? Just wondering if the persisted table growing is better than losing the stats data.

[maryliag]

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

---

-- commits line 13 at r1:

Previously, j82w (Jake) wrote…

What is the concern with the data blowing up? Just wondering if the persisted table growing is better than losing the stats data.

the problem is that we saw the case of getting 4Tb of data, and if the user wants the limit of 1M, that is a lot more.
After that we can work on improvements such as check the status of cleanup job to give warning or combining with flush, and this would no longer be an issue. But at least is a safety for now

[j82w]

-- commits line 13 at r1:

Previously, maryliag (Marylia Gutierrez) wrote…

the problem is that we saw the case of getting 4Tb of data, and if the user wants the limit of 1M, that is a lot more.
After that we can work on improvements such as check the status of cleanup job to give warning or combining with flush, and this would no longer be an issue. But at least is a safety for now

Even with 1M the UI will still fail to load right? Just wondering if we should focus on reducing the response size limit or do some change so that the UI always loads.

[xinhaoz]

pkg/sql/sqlstats/persistedsqlstats/flush.go line 49 at r1 (raw file):

	shouldWipeInMemoryStats = shouldWipeInMemoryStats || (!enabled && allowDiscardWhenDisabled)

	if shouldWipeInMemoryStats {

Do we need to change this behaviour at all? What did we do for in-memory stats prior to persisted stats? It would be nice if it behaved like a FIFO cache so that we could hold on to the in-memory stats between flush attempts.

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w and @xinhaoz)

---

-- commits line 13 at r1:

Previously, j82w (Jake) wrote…

Even with 1M the UI will still fail to load right? Just wondering if we should focus on reducing the response size limit or do some change so that the UI always loads.

Yes, it does fail to load, but that is not what this PR is trying to address. That would require more significant changes. Here I'm just focusing on the case where the cleanup gets stuck, fails, and I'm adding the safety to avoid data blowing up.

---

pkg/sql/sqlstats/persistedsqlstats/flush.go line 49 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Do we need to change this behaviour at all? What did we do for in-memory stats prior to persisted stats? It would be nice if it behaved like a FIFO cache so that we could hold on to the in-memory stats between flush attempts.

I'm still deciding if I should change this or not. I think I might keep as is for this PR, to focus on the safety check, and further improvements can look into this better. Specially if we want to change to do a cleanup along with flush, this behaviour will certainly change.

[ericharmeling]

LGTM as a safety check! I had a minor question/suggestion, but that could also wait for the follow-up PR.

Reviewed 2 of 3 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @j82w, @maryliag, and @xinhaoz)

---

pkg/sql/sqlstats/persistedsqlstats/flush.go line 104 at r2 (raw file):

	}
	actualSize := float64(tree.MustBeDInt(row[0]))
	return actualSize > (maxPersistedRows * 1.5)

The 1.5 multiplier seems a bit arbitrary. Would it make more sense to use sql.metrics.max_mem_stmt_fingerprints (i.e., up to one extra flush)?

Code quote:

1.5

[maryliag]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ericharmeling, @j82w, and @xinhaoz)

---

pkg/sql/sqlstats/persistedsqlstats/flush.go line 104 at r2 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…

The 1.5 multiplier seems a bit arbitrary. Would it make more sense to use sql.metrics.max_mem_stmt_fingerprints (i.e., up to one extra flush)?

My thinking for this number was:
with the default for memory 100k and default for persisted 1M, imagine that during each flush we do send 100k, and we do want to keep sending those until we reach the next hourly cleanup, so when you reach the 50min you would have added 500k extra, then on the hour you're about the clean, so I stop introducing more data.
Allowing just the max for the memory as extra is more conservative, but I wanted to give the extra room for data, but still not letting blowing up. I think it would be okay so let the data reach 1.5 of its max, knowing it will get cleanup soon.
We can always change this value if we receive any feedback on it off course.

[maryliag]

TFTR!
bors r+

[j82w]

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

---

pkg/sql/sqlstats/persistedsqlstats/flush.go line 85 at r2 (raw file):

	maxPersistedRows := float64(SQLStatsMaxPersistedRows.Get(&s.SQLStats.GetClusterSettings().SV))

	readStmt := `

Do we need to do any perf testing on this? I've noticed sometimes querying this table can take several seconds to over a minute. Would it be more efficient to get a descriptor and get a rough disk size or the table stats or directly query the time series db?

[craig]

Build failed:

• Bazel Essential CI (Cockroach)

[maryliag]

bors r-

[maryliag]

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[blathers-crl]

Encountered an error creating backports. Some common things that can go wrong:

1. The backport branch might have already existed.
2. There was a merge conflict.
3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

---

error creating merge commit from 5f357ba to blathers/backport-release-22.1-97123: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.1.x failed. See errors above.

---

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