sql: crdb_internal.reset_sql_stats() now resets persisted SQL Stats by Azhng · Pull Request #69273 · cockroachdb/cockroach

Azhng · 2021-08-24T00:46:06Z

Previously, crdb_internal.reset_sql_stats() builtin only resets
cluster-wide in-memory sql stats.
This patch updated the builtin to be able to reset persisted
sql stats as well.

Release justification: category 4

Release note (sql change): crdb_internal.reset_sql_stats() now resets
persisted SQL Stats.

cockroach-teamcity · 2021-08-24T00:46:29Z

This change is

Azhng · 2021-08-24T16:17:11Z

Reviewer note: only the last commit is the change.

matthewtodd

Reviewed 1 of 1 files at r1, 28 of 28 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @dt)

pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go, line 65 at r2 (raw file):

	compactionSchedule.SetOwner(security.NodeUserName())

	any, err := pbtypes.MarshalAny(&ScheduledSQLStatsCompactorExecutionArgs{})

nit: Maybe call this args or marshalledArgs or something?

pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go, line 142 at r2 (raw file):

	if err != nil {
		return exists, err

Would it make sense to use a literal false for this exists value, and the ones below? I'm not sure what idiomatic style is, but this confused me a bit -- I read through the function looking for a line assigning to exists but didn't find it. (I get how it works now, you're relying on the default value of bools being false.)

pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go, line 153 at r2 (raw file):

	}

	return tree.MustBeDInt(row[0]) == 1, nil /* err */

I suspect it would be safer / more defensive to say tree.MustBeDInt(row[0]) > 0 here.

Azhng

btw @matthewtodd the PR for the compaction schedule is #68401. This PR rebase off #68401 and the only relevant change the last commit.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @matthewtodd)

pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go, line 142 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Would it make sense to use a literal false for this exists value, and the ones below? I'm not sure what idiomatic style is, but this confused me a bit -- I read through the function looking for a line assigning to exists but didn't find it. (I get how it works now, you're relying on the default value of bools being false.)

Done.

pkg/sql/sqlstats/persistedsqlstats/compaction_scheduling.go, line 153 at r2 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

I suspect it would be safer / more defensive to say tree.MustBeDInt(row[0]) > 0 here.

Done.

matthewtodd

Oops, sorry. I'm confused by Reviewable & stacked PRs.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dt and @matthewtodd)

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @matthewtodd)

pkg/sql/sqlstats/persistedsqlstats/controller.go, line 62 at r4 (raw file):

// ResetClusterSQLStats implements the tree.SQLStatsController interface.
func (s *Controller) ResetClusterSQLStats(ctx context.Context) error {
	if err := s.Controller.ResetClusterSQLStats(ctx); err != nil {

is this part handling the in-memory and the remaining the disk? Can you add some comments?
I want to be clear that the reset is clearing both in-memory and on disk

Azhng

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

pkg/sql/sqlstats/persistedsqlstats/controller.go, line 62 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

is this part handling the in-memory and the remaining the disk? Can you add some comments?
I want to be clear that the reset is clearing both in-memory and on disk

Added comment.

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 42 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: remove extra space

Done.

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 60 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

this one is suppose to reset only disk stats? If this one is also resetting in memory, can you create a test that runs a few querys -> flush -> run a few more -> reset

Done.

maryliag

Reviewed 1 of 28 files at r4, 2 of 33 files at r5, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @matthewtodd)

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 60 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Done.

is it possible to check if the in-memory values are also 0 after the reset?

Azhng

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

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 60 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

is it possible to check if the in-memory values are also 0 after the reset?

Since we are checking crdb_internal views, we are checking both in-memory and persisted stats.

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng and @matthewtodd)

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 60 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Since we are checking crdb_internal views, we are checking both in-memory and persisted stats.

In this case, I would prefer if you have a different count for querys in-memory and disk, and check after that, so:

execute 3 querys -> run flush -> execute 2 querys (or any other number, you can even run the tests 2 other times, so you would have 6 in-memory ) -> check count = 5 (or 9 if running tests twice) -> reset -> check count = 0

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 83 at r7 (raw file):

	for _, row := range result {
		_, found := testCases[row[0]]
		require.True(t, found, "expect %s to be found, not it was not", row[0])

nit: but it was not (or something like this)

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 106 at r7 (raw file):

	for _, row := range result {
		_, found := testCases[row[0]]
		require.True(t, found, "expect %s to be found, not it was not", row[0])

nit: but

Azhng

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

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 60 at r4 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

In this case, I would prefer if you have a different count for querys in-memory and disk, and check after that, so:

execute 3 querys -> run flush -> execute 2 querys (or any other number, you can even run the tests 2 other times, so you would have 6 in-memory ) -> check count = 5 (or 9 if running tests twice) -> reset -> check count = 0

Updated tests to have different numbers of in-memory stats (3) and persisted stats (4) with overlapping fingerprints.
Also updated the test to instead of check for counts, it perform deep inspection on the statement fingerprints and fingerprint IDs.

maryliag

Just one small nit and make sure the tests are passing, otherwise

Reviewed all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng and @matthewtodd)

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 60 at r4 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Updated tests to have different numbers of in-memory stats (3) and persisted stats (4) with overlapping fingerprints.
Also updated the test to instead of check for counts, it perform deep inspection on the statement fingerprints and fingerprint IDs.

Great improvement, thanks!

pkg/sql/sqlstats/persistedsqlstats/controller_test.go, line 129 at r9 (raw file):

				found = true

				// Populate fingerprintID

nit: period

Azhng · 2021-08-27T20:51:06Z

@maryliag, found out the cause for the unit test timeout. The culprit is TestLogic/5node/distsql_crdb_internal. The offending query was originally introduced as a regression test for #62587.

Since the crdb_internal.reset_sql_stats() is now using TRUNCATE, it has become a lot more expensive to execute. Therefore the offending query now effectively performs 10000 truncate on our own system table 🤦 .

We created a small table so that this executes a lot faster.

PTAL

maryliag

can this cause an issue on production or was the issue just for testing? meaning, if there is a huge number of rows in reset, could we hit the timeout there too?

Reviewed 1 of 33 files at r5, 1 of 1 files at r10, 2 of 2 files at r11, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @Azhng and @matthewtodd)

Azhng · 2021-08-27T21:30:23Z

I can't imagine anyone in production writing queries like that. It's like doing

SELECT * FROM table WHERE crdb_internal.reset_sql_stats()

The query in #62587 was randomly generated by SQLSmith (a sort-of random SQL query generator).

maryliag

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @Azhng, @maryliag, and @matthewtodd)

Previously, crdb_internal.reset_sql_stats() builtin only resets cluster-wide in-memory sql stats. This patch updated the builtin to be able to reset persisted sql stats as well. Release justification: category 4 Release note (sql change): crdb_internal.reset_sql_stats() now resets persisted SQL Stats.

Azhng · 2021-08-28T05:06:57Z

TFTR!

bors r=maryliag

craig · 2021-08-28T08:50:21Z

Build failed (retrying...):

GitHub CI (Cockroach)

craig · 2021-08-28T09:33:25Z

Build succeeded:

GitHub CI (Cockroach)

Azhng mentioned this pull request Aug 24, 2021

sql: persistent SQL Stats main tracking issue #64743

Closed

24 tasks

Azhng force-pushed the sqlstats-reset branch 2 times, most recently from c7f73ed to 7a88fb5 Compare August 24, 2021 16:16

Azhng force-pushed the sqlstats-reset branch 3 times, most recently from e6a2dbf to 4751895 Compare August 25, 2021 02:21

Azhng requested a review from a team August 25, 2021 02:21

Azhng marked this pull request as ready for review August 25, 2021 02:21

Azhng requested a review from a team August 25, 2021 02:21

Azhng requested a review from a team as a code owner August 25, 2021 02:21

Azhng requested review from a team and dt and removed request for a team August 25, 2021 02:21

matthewtodd suggested changes Aug 25, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from 4751895 to 0dc3b81 Compare August 25, 2021 17:59

Azhng commented Aug 25, 2021

View reviewed changes

matthewtodd reviewed Aug 25, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from 0dc3b81 to f42ed5a Compare August 26, 2021 04:51

Azhng removed request for a team August 26, 2021 04:53

Azhng force-pushed the sqlstats-reset branch 3 times, most recently from 6353d03 to fa0fca3 Compare August 26, 2021 18:12

Azhng requested a review from a team as a code owner August 26, 2021 18:12

Azhng removed request for a team and dt August 26, 2021 19:45

Azhng force-pushed the sqlstats-reset branch from fa0fca3 to ac9c320 Compare August 27, 2021 00:04

maryliag reviewed Aug 27, 2021

View reviewed changes

Azhng commented Aug 27, 2021

View reviewed changes

maryliag reviewed Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from 1f0dec0 to 444e90e Compare August 27, 2021 15:11

Azhng commented Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from 444e90e to 092fe01 Compare August 27, 2021 15:16

maryliag suggested changes Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from 092fe01 to 92bca31 Compare August 27, 2021 16:35

Azhng commented Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from 92bca31 to 5d05611 Compare August 27, 2021 16:39

maryliag approved these changes Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch 3 times, most recently from 885a0f6 to b4fea5e Compare August 27, 2021 20:46

Azhng requested a review from maryliag August 27, 2021 20:51

maryliag reviewed Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch from b4fea5e to c37814b Compare August 27, 2021 21:38

maryliag approved these changes Aug 27, 2021

View reviewed changes

Azhng force-pushed the sqlstats-reset branch 2 times, most recently from f94d850 to d0406b7 Compare August 28, 2021 03:58

Azhng force-pushed the sqlstats-reset branch from d0406b7 to 0a1801d Compare August 28, 2021 04:00

craig bot merged commit e016cd6 into cockroachdb:master Aug 28, 2021

cockroach-teamcity mentioned this pull request Aug 28, 2021

sql: crdb_internal.reset_sql_stats() now resets persisted SQL Stats cockroachdb/docs#11160

Closed

jseldess mentioned this pull request Sep 8, 2021

sql: crdb_internal.reset_sql_stats() now resets persisted SQL Stats cockroachdb/docs#11459

Closed

Conversation

Azhng commented Aug 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cockroach-teamcity commented Aug 24, 2021

Uh oh!

Azhng commented Aug 24, 2021

Uh oh!

matthewtodd left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

matthewtodd left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng left a comment

Choose a reason for hiding this comment

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng commented Aug 27, 2021

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng commented Aug 27, 2021

Uh oh!

maryliag left a comment

Choose a reason for hiding this comment

Uh oh!

Azhng commented Aug 28, 2021

Uh oh!

craig bot commented Aug 28, 2021

Uh oh!

craig bot commented Aug 28, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Azhng commented Aug 24, 2021 •

edited

Loading