Skip to content

sql/logictest: disable stats collection for system tables earlier#99155

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
rytaft:deflake-sql_statistics_persisted
Mar 22, 2023
Merged

sql/logictest: disable stats collection for system tables earlier#99155
craig[bot] merged 1 commit intocockroachdb:masterfrom
rytaft:deflake-sql_statistics_persisted

Conversation

@rytaft
Copy link
Copy Markdown
Collaborator

@rytaft rytaft commented Mar 21, 2023

This commit updates the logictests to disable stats collection for system tables before the test cluster is started. This avoids a race condition where stats might be collected on system tables before collection is disabled with SET CLUSTER SETTING.

This should prevent flakes for tests that show EXPLAIN output for queries over system tables.

Fixes #99118

Release note: None

@rytaft rytaft requested review from a team, msirek and yuzefovich March 21, 2023 21:02
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Member

@yuzefovich yuzefovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: Thanks!

Maybe also revert #97775 then?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @msirek)

Copy link
Copy Markdown
Contributor

@msirek msirek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


pkg/sql/logictest/logic.go line 1460 at r1 (raw file):

	stats.AutomaticStatisticsOnSystemTables.Override(
		context.Background(), &params.ServerArgs.TempStorageConfig.Settings.SV, false,
	)

Should we do this for sql.stats.automatic_collection.enabled as well, which is also using SET CLUSTER SETTING below?

@rytaft rytaft force-pushed the deflake-sql_statistics_persisted branch from a160c61 to 7f7fc3e Compare March 21, 2023 21:13
Copy link
Copy Markdown
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTRs!

Maybe also revert #97775 then?

Done.

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


pkg/sql/logictest/logic.go line 1460 at r1 (raw file):

Previously, msirek (Mark Sirek) wrote…

Should we do this for sql.stats.automatic_collection.enabled as well, which is also using SET CLUSTER SETTING below?

It's not really needed for that one, since we won't have created any non-system tables by that point, so there's no race condition. I think I'll leave it there, if you don't mind, since there's a big comment and a bunch of other stats settings getting updated in the same place.

This commit updates the logictests to disable stats collection for system
tables before the test cluster is started. This avoids a race condition where
stats might be collected on system tables before collection is disabled with
SET CLUSTER SETTING.

This should prevent flakes for tests that show EXPLAIN output for queries over
system tables.

Fixes cockroachdb#99118

Release note: None
@rytaft rytaft force-pushed the deflake-sql_statistics_persisted branch from 7f7fc3e to 9c6fcd1 Compare March 21, 2023 21:18
Copy link
Copy Markdown
Collaborator Author

@rytaft rytaft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

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

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 22, 2023

Build failed (retrying...):

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 22, 2023

Build failed (retrying...):

@knz knz added the backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only label Mar 22, 2023
@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 22, 2023

Build succeeded:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-23.1.x PAST MAINTENANCE SUPPORT: 23.1 patch releases via ER request only

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pkg/sql/opt/exec/execbuilder/tests/local/local_test: TestExecBuild_sql_statistics_persisted failed

5 participants