persistedsqlstats: add AOST clause to statement statistics size check #107549

THardy98 · 2023-07-25T17:35:46Z

Epic: None

This change as an AOST clause to the statement statistics size check.
This helps mitigate contention on the system.statement_statistics
table which has caused the sql stats compaction job to fail.

Release note: None

cockroach-teamcity · 2023-07-25T17:35:55Z

This change is

maryliag

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

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

	if err != nil {
		log.Errorf(ctx, "encountered an error at flush, checking for statement statistics size limit: %v", err)
		fmt.Println("err", err)

I assume you added this print for debugging, so you can remove it

maryliag

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

-- commits line 8 at r1:
can you add a release note? you don't need to go into details about the AOST, but add that this is fixing the retry error on compaction job

j82w

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

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

	aggregatedTs := s.ComputeAggregatedTs()

	limitReached, err := s.StmtsLimitSizeReached(ctx)

nit: add a comment explaining that only stmt count needs to be checked because there should always be more stmts than txns.

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

		log.Errorf(ctx, "encountered an error at flush, checking for statement statistics size limit: %v", err)
		fmt.Println("err", err)
	} else if limitReached {

Before if an error occurred in the StmtsLimitSizeReached it returned false and still allowed the flush. Let's keep the same behavior.

xinhaoz · 2023-07-25T19:34:53Z

pkg/sql/sqlstats/persistedsqlstats/flush_test.go

+	defer s.Stopper().Stop(context.Background())
+	sqlConn := sqlutils.MakeSQLRunner(conn)
+	sqlConn.Exec(t, `INSERT INTO system.users VALUES ('node', NULL, true, 3); GRANT node TO root`)
+	waitForFollowerReadTimestamp(t, sqlConn)


Why can't you just set the testing knob to make the AOST time a more recent value? We usually use '-1us'.

This change as an AOST clause to the statement statistics size check. This helps mitigate contention on the `system.statement_statistics` table which has caused the sql stats compaction job to fail. Release note (bug fix): this change reduces contention on the `system.statement_statistics` table which has caused the SQL statistics compaction job to fail

THardy98

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

-- commits line 8 at r1:

Previously, maryliag (Marylia Gutierrez) wrote…

can you add a release note? you don't need to go into details about the AOST, but add that this is fixing the retry error on compaction job

Added one, labelled it as a bug fix.

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

Previously, j82w (Jake) wrote…

nit: add a comment explaining that only stmt count needs to be checked because there should always be more stmts than txns.

Done.

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

Previously, maryliag (Marylia Gutierrez) wrote…

I assume you added this print for debugging, so you can remove it

Removed.

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

Previously, j82w (Jake) wrote…

Before if an error occurred in the StmtsLimitSizeReached it returned false and still allowed the flush. Let's keep the same behavior.

Removed the conditional. If an error occurs, we just log it.

pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 539 at r1 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Why can't you just set the testing knob to make the AOST time a more recent value? We usually use '-1us'.

Considered it but after some offline discussion (here) opted to wait for the follower read timestamp to better replicate the code flow.

j82w

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

j82w · 2023-07-25T20:51:35Z

pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 576 at r2 (raw file):

	// (meaning that the limit will be reached) and no error. We need SucceedsSoon here for the follower
	// read timestamp to catch up enough for this state to be reached.
	testutils.SucceedsSoon(t, func() error {

How could this ever succeed if the statement above took an update lock? It shouldn't be possible to add rows right?

THardy98

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

pkg/sql/sqlstats/persistedsqlstats/flush_test.go line 576 at r2 (raw file):

Previously, j82w (Jake) wrote…

How could this ever succeed if the statement above took an update lock? It shouldn't be possible to add rows right?

It doesn't fail initially because there aren't enough rows in the table, it fails initially because the number of rows AOST follower_read_timestamp is less than the actual number of rows needed for limitReached == true. We SucceedsSoon to keep checking the rows until AOST catches up to see them all

maryliag · 2023-07-25T22:17:56Z

bors r+

craig · 2023-07-26T06:06:46Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-07-26T06:06:53Z

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

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

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

error creating merge commit from cead5f7 to blathers/backport-release-22.2-107549: 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.2.x failed. See errors above.

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

THardy98 requested a review from a team July 25, 2023 17:35

THardy98 marked this pull request as draft July 25, 2023 17:35

THardy98 force-pushed the update_fetch_stmt_count_as_follower_read branch 2 times, most recently from f58ef30 to 47adc69 Compare July 25, 2023 19:16

THardy98 changed the title ~~wip~~ persistedsqlstats: add AOST clause to statement statistics size check Jul 25, 2023

THardy98 marked this pull request as ready for review July 25, 2023 19:17

maryliag reviewed Jul 25, 2023

View reviewed changes

j82w reviewed Jul 25, 2023

View reviewed changes

xinhaoz reviewed Jul 25, 2023

View reviewed changes

THardy98 force-pushed the update_fetch_stmt_count_as_follower_read branch from 47adc69 to 5dc9995 Compare July 25, 2023 19:54

THardy98 force-pushed the update_fetch_stmt_count_as_follower_read branch from 5dc9995 to cead5f7 Compare July 25, 2023 19:59

THardy98 commented Jul 25, 2023

View reviewed changes

j82w approved these changes Jul 25, 2023

View reviewed changes

THardy98 mentioned this pull request Jul 25, 2023

release-23.1: persistedsqlstats: add AOST clause to statement statistics size check #107573

Merged

THardy98 added the backport-22.2.x label Jul 25, 2023

THardy98 commented Jul 25, 2023

View reviewed changes

craig bot merged commit 68985a2 into cockroachdb:master Jul 26, 2023

This was referenced Jul 27, 2023

sql: retry sql stats compaction job #107306

Closed

release-22.2: persistedsqlstats: add AOST clause to statement statistics size check #107794

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

persistedsqlstats: add AOST clause to statement statistics size check #107549

persistedsqlstats: add AOST clause to statement statistics size check #107549

THardy98 commented Jul 25, 2023 •

edited

Loading

cockroach-teamcity commented Jul 25, 2023

maryliag left a comment

maryliag left a comment

j82w left a comment

xinhaoz Jul 25, 2023

THardy98 left a comment

j82w left a comment

j82w commented Jul 25, 2023

THardy98 left a comment •

edited

Loading

maryliag commented Jul 25, 2023

craig bot commented Jul 26, 2023

blathers-crl bot commented Jul 26, 2023

persistedsqlstats: add AOST clause to statement statistics size check #107549

persistedsqlstats: add AOST clause to statement statistics size check #107549

Conversation

THardy98 commented Jul 25, 2023 • edited Loading

cockroach-teamcity commented Jul 25, 2023

maryliag left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

j82w left a comment

Choose a reason for hiding this comment

xinhaoz Jul 25, 2023

Choose a reason for hiding this comment

THardy98 left a comment

Choose a reason for hiding this comment

j82w left a comment

Choose a reason for hiding this comment

j82w commented Jul 25, 2023

THardy98 left a comment • edited Loading

Choose a reason for hiding this comment

maryliag commented Jul 25, 2023

craig bot commented Jul 26, 2023

blathers-crl bot commented Jul 26, 2023

THardy98 commented Jul 25, 2023 •

edited

Loading

THardy98 left a comment •

edited

Loading