sql: fix incorrect follower read and testing knob for sql stats compaction job #69346

Azhng · 2021-08-25T01:25:38Z

Previously, the SQL Stats Compaction job did not check for nil testing
knobs and constructed its SQL query with incorrect follower read syntax.
This commit address those two bugs.

Release justification: Category 2: Bug fixes and low-risk updates
to new functionality

Release note: None

cockroach-teamcity · 2021-08-25T01:25:47Z

This change is

maryliag

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

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

SELECT count(*)
FROM %[1]s
%[3]s

why it was necessary to change to order of this clause?

matthewtodd

Reviewed 1 of 1 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @Azhng)

-- commits, line 5 at r1:
nit: spelling / grammar: "Previously, the SQL Stats Compaction job did not check for nil testing knobs and constructed its SQL query with incorrect follower read syntax."

-- commits, line 11 at r1:
über-nit: leading space

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

SELECT count(*)
FROM %[1]s
%[3]s

Is it possible to write a test for this fix?

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

SELECT count(*)
FROM %[1]s
%[3]s

And would you want to reorder the placeholder indexes now to match their positions?

SELECT count(*)
FROM %[1]s
%[2]s
WHERE %[3]s = $1

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 204 at r1 (raw file):

	followerReadClause := "AS OF SYSTEM TIME follower_read_timestamp()"

	if c.knobs != nil && c.knobs.DisableFollowerRead {

Is it possible to write a test for this fix? Or, better yet, is there a way to make these knobs never nil in the first place?

matthewtodd

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

Azhng

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

-- commits, line 5 at r1:

Previously, matthewtodd (Matthew Todd) wrote…

nit: spelling / grammar: "Previously, the SQL Stats Compaction job did not check for nil testing knobs and constructed its SQL query with incorrect follower read syntax."

Done.

-- commits, line 11 at r1:

Previously, matthewtodd (Matthew Todd) wrote…

über-nit: leading space

Done.

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

why it was necessary to change to order of this clause?

This is because the ASOT clause was placed in a wrong place.

Originally, it generates queries like:

SELECT count(*)
FROM system.statement_statistics
WHERE ...
AS OF SYSTEM TIME follower_read_timestamp()

but the correct syntax should be

SELECT count(*)
FROM system.statement_statistics
AS OF SYSTEM TIME follower_read_timestamp()
WHERE ...

We disabled follower read in test because this would cause historical reads into a point where system tables are not even created yet. This is a scenario where it doesn't happen outside of testing.

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Is it possible to write a test for this fix?

Would be difficult, follower read is difficult for us to test in the SQL land. The only hack would be time.Sleep() and wait for the follower read timestamp to be higher than the point where system tables are created. But we really want to avoid doing those in test.

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

And would you want to reorder the placeholder indexes now to match their positions?
SELECT count(*)
FROM %[1]s
%[2]s
WHERE %[3]s = $1

Done.

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 204 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Is it possible to write a test for this fix? Or, better yet, is there a way to make these knobs never nil in the first place?

Hmm, since this is only used for test, I hope we can avoid the extra allocation unless it's necessary.

matthewtodd

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

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Would be difficult, follower read is difficult for us to test in the SQL land. The only hack would be time.Sleep() and wait for the follower read timestamp to be higher than the point where system tables are created. But we really want to avoid doing those in test.

Understood. It's a shame we can't protect ourselves from bad SQL. Like maybe we'd want some infrastructure to run the query through the parser or something. But non-blocking for now.

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 204 at r1 (raw file):

Previously, Azhng (Archer Zhang) wrote…

Hmm, since this is only used for test, I hope we can avoid the extra allocation unless it's necessary.

Fair enough. :-) Thoughts on a test somehow?

Azhng

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

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 204 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Fair enough. :-) Thoughts on a test somehow?

Done.

matthewtodd

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

maryliag

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

Azhng · 2021-08-26T13:33:30Z

TFTR!

bors r=maryliag,mathewtodd

ajwerner

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

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

Understood. It's a shame we can't protect ourselves from bad SQL. Like maybe we'd want some infrastructure to run the query through the parser or something. But non-blocking for now.

You could pass a different expression in the follower read clause like '-1us' and then sleep for 1 microsecond. I prefer that to no testing.

ajwerner

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

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, ajwerner wrote…

You could pass a different expression in the follower read clause like '-1us' and then sleep for 1 microsecond. I prefer that to no testing.

In fact, maybe that's the answer here. Maybe we get rid of the knob to disable follower reads and replace it with a knob to set up the fixed timestamp to '-1us'. I don't think you need to sleep 1us.

Azhng · 2021-08-26T13:58:02Z

bors r-

craig · 2021-08-26T13:58:04Z

Canceled.

Azhng

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

pkg/sql/sqlstats/persistedsqlstats/compaction_exec.go, line 199 at r1 (raw file):

Previously, ajwerner wrote…

In fact, maybe that's the answer here. Maybe we get rid of the knob to disable follower reads and replace it with a knob to set up the fixed timestamp to '-1us'. I don't think you need to sleep 1us.

Done.

ajwerner

🚢 it

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

pkg/sql/sqlstats/persistedsqlstats/reader_test.go, line 57 at r5 (raw file):

	// Wait for the system table to be created by ASOT timestamp.
	time.Sleep(time.Microsecond)

Do you actually need these? I have a feeling the answer is no.

matthewtodd

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

pkg/sql/sqlstats/persistedsqlstats/test_utils.go, line 27 at r5 (raw file):

	// ASOTClause overrides the AS OF SYSTEM TIME clause in queries used in
	// persistedsqlstats.
	ASOTClause string

nit: Do you want this to be AOSTClause instead?

ajwerner · 2021-08-26T15:17:23Z

I don't know what it is about the acronym AOST but it has been written accidentally as ASOT a lot, including by myself. My brain, for whatever reason, doesn't see the transposition. A simple slack search shows ASOT having been written 21 times by 7 different people 🤯

matthewtodd · 2021-08-26T15:20:24Z

Amazing. 🤯

Azhng

Human brain is interesting 🤯

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

pkg/sql/sqlstats/persistedsqlstats/reader_test.go, line 57 at r5 (raw file):

Previously, ajwerner wrote…

	// Wait for the system table to be created by ASOT timestamp.
	time.Sleep(time.Microsecond)
Do you actually need these? I have a feeling the answer is no.

Removed.

pkg/sql/sqlstats/persistedsqlstats/test_utils.go, line 27 at r5 (raw file):

Previously, matthewtodd (Matthew Todd) wrote…

nit: Do you want this to be AOSTClause instead?

Done.

compaction job Previously, the SQL Stats Compaction job did not check for nil testing knobs and constructed its SQL query with incorrect follower read syntax This commit address those two bugs. Release justification: Category 2: Bug fixes and low-risk updates to new functionality Release note: None

Azhng · 2021-08-26T21:30:52Z

TFTR!

bors r=ajwerner,maryliag,matthewtodd

craig · 2021-08-26T23:11:11Z

Build succeeded:

GitHub CI (Cockroach)

68401: sql: create sql stats compaction scheduled job r=miretskiy,maryliag a=Azhng Depends on #68434, #69346 sql: create sql stats compaction scheduled job This commit introduces SQL Stats Compaction Scheduled job, where it runs in an hourly schedule and invokes the SQL Stats Compaction Job that was created in the previous commit. This commit also introduces `crdb_internal.schedule_sql_stats_compaction()` builtin as a way to manually create SQL Stats compaction schedule. `SHOW SCHEDULES` command'syntax is also extended to support `SHOW SCHEDUELS FOR SQL STATISTICS`. Release justification: category 4 Release note (sql change): introduce crdb_internal.schedule_sql_stats_compaction() to manually create SQL Stats compaction schedule. Extend SHOW SCHEDULES command to support SHOW SCHEDULES FOR SQL STATISTICS. Co-authored-by: Azhng <archer.xn@gmail.com>

Azhng requested a review from a team August 25, 2021 01:25

Azhng force-pushed the sqlstats-fix-followerread branch from 7647f12 to 5f19d4f Compare August 25, 2021 01:26

This was referenced Aug 25, 2021

sql: create sql stats compaction scheduled job #68401

Merged

sql: persistent SQL Stats main tracking issue #64743

Closed

maryliag reviewed Aug 25, 2021

View reviewed changes

matthewtodd suggested changes Aug 25, 2021

View reviewed changes

matthewtodd reviewed Aug 25, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from 5f19d4f to 46a0cc9 Compare August 25, 2021 16:45

Azhng commented Aug 25, 2021

View reviewed changes

matthewtodd suggested changes Aug 25, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from 46a0cc9 to 19332f7 Compare August 25, 2021 17:51

Azhng commented Aug 25, 2021

View reviewed changes

matthewtodd approved these changes Aug 25, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from 19332f7 to 8583ec4 Compare August 26, 2021 02:38

maryliag approved these changes Aug 26, 2021

View reviewed changes

ajwerner reviewed Aug 26, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from 8583ec4 to dfdd8ea Compare August 26, 2021 14:59

Azhng commented Aug 26, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from dfdd8ea to fab0c78 Compare August 26, 2021 15:02

ajwerner approved these changes Aug 26, 2021

View reviewed changes

matthewtodd reviewed Aug 26, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from fab0c78 to 0f6224f Compare August 26, 2021 15:23

Azhng commented Aug 26, 2021

View reviewed changes

Azhng force-pushed the sqlstats-fix-followerread branch from 0f6224f to 0a29bea Compare August 26, 2021 18:10

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

Azhng removed the request for review from a team August 26, 2021 19:13

Azhng force-pushed the sqlstats-fix-followerread branch from 0a29bea to 5464b74 Compare August 26, 2021 19:20

Azhng force-pushed the sqlstats-fix-followerread branch from 5464b74 to 2fad679 Compare August 26, 2021 20:04

craig bot merged commit 7d94aa8 into cockroachdb:master Aug 26, 2021

dhartunian mentioned this pull request Sep 14, 2021

ccl/serverccl: TestTenantGRPCServices failed #69948

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql: fix incorrect follower read and testing knob for sql stats compaction job #69346

sql: fix incorrect follower read and testing knob for sql stats compaction job #69346

Azhng commented Aug 25, 2021 •

edited

Loading

cockroach-teamcity commented Aug 25, 2021

maryliag left a comment

matthewtodd left a comment

matthewtodd left a comment

Azhng left a comment

matthewtodd left a comment

Azhng left a comment

matthewtodd left a comment

maryliag left a comment

Azhng commented Aug 26, 2021

ajwerner left a comment

ajwerner left a comment

Azhng commented Aug 26, 2021

craig bot commented Aug 26, 2021

Azhng left a comment

ajwerner left a comment

matthewtodd left a comment

ajwerner commented Aug 26, 2021

matthewtodd commented Aug 26, 2021

Azhng left a comment

Azhng commented Aug 26, 2021

craig bot commented Aug 26, 2021

sql: fix incorrect follower read and testing knob for sql stats compaction job #69346

sql: fix incorrect follower read and testing knob for sql stats compaction job #69346

Conversation

Azhng commented Aug 25, 2021 • edited Loading

cockroach-teamcity commented Aug 25, 2021

maryliag left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

Azhng left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

Azhng commented Aug 26, 2021

ajwerner left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

Azhng commented Aug 26, 2021

craig bot commented Aug 26, 2021

Azhng left a comment

Choose a reason for hiding this comment

ajwerner left a comment

Choose a reason for hiding this comment

matthewtodd left a comment

Choose a reason for hiding this comment

ajwerner commented Aug 26, 2021

matthewtodd commented Aug 26, 2021

Azhng left a comment

Choose a reason for hiding this comment

Azhng commented Aug 26, 2021

craig bot commented Aug 26, 2021

Azhng commented Aug 25, 2021 •

edited

Loading