sql/stats: don't use linear regression with NaN for stats forecasting #113712

DrewKimball · 2023-11-02T21:12:13Z

This patch adds a method to the quantile function, invalid, which checks for NaN values within the function. These indicate a failure to derive a linear regression model for the observed histograms, and can lead to internal errors and panics if not caught. Stats forecasting now checks this method before attempting to use a quantile function to derive a histogram prediction.

Fixes #113645
Fixes #113680

Release note (bug fix): Fixed a bug that could cause internal errors or panics while attempting to forecast statistics on a numeric column.

cockroach-teamcity · 2023-11-02T21:12:23Z

This change is

yuzefovich

I'll let Michael review this, but I wonder whether we want to revert 120132e given that we now have a proper fix?

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

yuzefovich

Nice job finding this btw!

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

DrewKimball

Thanks!

I wonder whether we want to revert 120132e given that we now have a proper fix?

Good point. I've reverted it, since fixMalformed shouldn't return an error now.

I also expanded the invalid check to include +/-Inf values.

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

michae2

Gah, I was checking for these in toQuantileValue and fromQuantileValue but it never occurred to me to check in between! Great catch!

It's possible this fixes #93892 as well, if we have an invalid quantile that makes it through fixMalformed without panicking. I'll think about that more.

@DrewKimball WDYT about adding another commit that adds panic-catching to GetTableStats?

cockroach/pkg/sql/stats/stats_cache.go

Lines 215 to 223 in 722f902

    
           func (sc *TableStatisticsCache) GetTableStats( 
        
           	ctx context.Context, table catalog.TableDescriptor, 
        
           ) ([]*TableStatistic, error) { 
        
           	if !statsUsageAllowed(table, sc.settings) { 
        
           		return nil, nil 
        
           	} 
        
           	forecast := forecastAllowed(table, sc.settings) 
        
           	return sc.getTableStatsFromCache(ctx, table.GetID(), &forecast) 
        
           }

If we panic while getting stats, I think we should try to plan the query without stats, rather than cause the query (all queries touching that table) to fail.

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @DrewKimball)

pkg/sql/stats/quantile.go line 658 at r2 (raw file):

// This function fixes the negative slope pieces by "moving" p from the
// overlapping places to where it should be. No p is lost in the making of these
// calculations.

If you wouldn't mind, could you add a sentence to this comment mentioning that isInvalid must be checked first?

This patch adds a method to the quantile function, `invalid`, which checks for `NaN` and `+/-Inf` values within the function. These indicate a failure to derive a linear regression model for the observed histograms, and can lead to internal errors and panics if not caught. Stats forecasting now checks this method before attempting to use a quantile function to derive a histogram prediction. This patch also reverts 120132e, since `fixMalformed` is no longer expected to return an error. Fixes cockroachdb#113645 Fixes cockroachdb#113680 Release note (bug fix): Fixed a bug that could cause internal errors or panics while attempting to forecast statistics on a numeric column.

This patch adds panic-catching logic to `GetTableStats`, so that "safe" panics (out-of-bounds, assertion etc.) can be propagated as a returned error. This allows some code-paths to ignore the returned error, so that execution can proceed even there's an unexpected error in the stats code. This prevents situations where it becomes impossible to query a table because of a stats bug. Informs cockroachdb#113645 Release note: None

DrewKimball

If we panic while getting stats, I think we should try to plan the query without stats, rather than cause the query (all queries touching that table) to fail.

I think that makes sense, done.

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

pkg/sql/stats/quantile.go line 658 at r2 (raw file):

Previously, michae2 (Michael Erickson) wrote…

If you wouldn't mind, could you add a sentence to this comment mentioning that isInvalid must be checked first?

Done.

michae2

Thank you!

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

DrewKimball · 2023-11-03T21:46:36Z

TFTR!

bors r+

craig · 2023-11-03T23:13:10Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl · 2023-11-03T23:13:27Z

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 050b37c to blathers/backport-release-22.2-113712: 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.

error creating merge commit from 050b37c to blathers/backport-release-23.1-113712: 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 23.1.x failed. See errors above.

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

michae2 · 2023-11-03T23:29:54Z

I think we should also backport this to release-22.2.17-rc

DrewKimball · 2023-11-04T01:13:53Z

blathers backport 23.1.12-rc

blathers-crl · 2023-11-04T01:13:57Z

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 050b37c to blathers/backport-release-23.1.12-rc.FROZEN-113712: 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 23.1.12-rc failed. See errors above.

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

DrewKimball · 2023-11-06T17:23:57Z

blathers backport 23.1.12-rc.Frozen

blathers-crl · 2023-11-06T17:24:00Z

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 getting backport branch release-23.1.12-rc.Frozen: unexpected status code: 404 Not Found

Backport to branch 23.1.12-rc.Frozen failed. See errors above.

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

DrewKimball · 2023-11-06T17:24:31Z

blathers backport 23.1.12-rc.FROZEN

blathers-crl · 2023-11-06T17:24:35Z

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 050b37c to blathers/backport-release-23.1.12-rc.FROZEN-113712: 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 23.1.12-rc.FROZEN failed. See errors above.

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

DrewKimball added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 backport-23.2.x Flags PRs that need to be backported to 23.2. labels Nov 2, 2023

DrewKimball requested a review from michae2 November 2, 2023 21:12

DrewKimball requested a review from a team as a code owner November 2, 2023 21:12

yuzefovich reviewed Nov 2, 2023

View reviewed changes

DrewKimball force-pushed the quantile branch from 50a042d to dbd74be Compare November 2, 2023 22:00

DrewKimball commented Nov 2, 2023

View reviewed changes

michae2 approved these changes Nov 2, 2023

View reviewed changes

DrewKimball added 2 commits November 3, 2023 02:15

DrewKimball force-pushed the quantile branch from dbd74be to 3458ada Compare November 3, 2023 08:29

DrewKimball commented Nov 3, 2023

View reviewed changes

michae2 approved these changes Nov 3, 2023

View reviewed changes

craig bot merged commit f6d6a9d into cockroachdb:master Nov 3, 2023

blathers-crl bot mentioned this pull request Nov 3, 2023

release-23.2: sql/stats: don't use linear regression with NaN for stats forecasting #113797

Merged

DrewKimball deleted the quantile branch November 4, 2023 00:03

DrewKimball mentioned this pull request Nov 6, 2023

release-23.1.12-rc.FROZEN: sql/stats: don't use linear regression with NaN for stats forecasting #113881

Merged

michae2 mentioned this pull request Jan 24, 2024

crash: internal error: the first bucket should have NumRange=0 #93892

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sql/stats: don't use linear regression with NaN for stats forecasting #113712

sql/stats: don't use linear regression with NaN for stats forecasting #113712

DrewKimball commented Nov 2, 2023

cockroach-teamcity commented Nov 2, 2023

yuzefovich left a comment

yuzefovich left a comment

DrewKimball left a comment

michae2 left a comment

DrewKimball left a comment

michae2 left a comment

DrewKimball commented Nov 3, 2023

craig bot commented Nov 3, 2023

blathers-crl bot commented Nov 3, 2023

michae2 commented Nov 3, 2023

DrewKimball commented Nov 4, 2023

blathers-crl bot commented Nov 4, 2023

DrewKimball commented Nov 6, 2023

blathers-crl bot commented Nov 6, 2023

DrewKimball commented Nov 6, 2023

blathers-crl bot commented Nov 6, 2023

	func (sc *TableStatisticsCache) GetTableStats(
	ctx context.Context, table catalog.TableDescriptor,
	) ([]*TableStatistic, error) {
	if !statsUsageAllowed(table, sc.settings) {
	return nil, nil
	}
	forecast := forecastAllowed(table, sc.settings)
	return sc.getTableStatsFromCache(ctx, table.GetID(), &forecast)
	}

sql/stats: don't use linear regression with NaN for stats forecasting #113712

sql/stats: don't use linear regression with NaN for stats forecasting #113712

Conversation

DrewKimball commented Nov 2, 2023

cockroach-teamcity commented Nov 2, 2023

yuzefovich left a comment

Choose a reason for hiding this comment

yuzefovich left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

DrewKimball left a comment

Choose a reason for hiding this comment

michae2 left a comment

Choose a reason for hiding this comment

DrewKimball commented Nov 3, 2023

craig bot commented Nov 3, 2023

blathers-crl bot commented Nov 3, 2023

michae2 commented Nov 3, 2023

DrewKimball commented Nov 4, 2023

blathers-crl bot commented Nov 4, 2023

DrewKimball commented Nov 6, 2023

blathers-crl bot commented Nov 6, 2023

DrewKimball commented Nov 6, 2023

blathers-crl bot commented Nov 6, 2023