Skip to content

Commit

Permalink
sql/stats: add panic-catching to GetTableStats
Browse files Browse the repository at this point in the history
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 #113645

Release note: None
  • Loading branch information
DrewKimball committed Nov 6, 2023
1 parent 1afac4f commit 5d0e071
Showing 1 changed file with 15 additions and 1 deletion.
16 changes: 15 additions & 1 deletion pkg/sql/stats/stats_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql/types"
"github.com/cockroachdb/cockroach/pkg/util/cache"
"github.com/cockroachdb/cockroach/pkg/util/encoding"
"github.com/cockroachdb/cockroach/pkg/util/errorutil"
"github.com/cockroachdb/cockroach/pkg/util/hlc"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/protoutil"
Expand Down Expand Up @@ -214,10 +215,23 @@ func decodeTableStatisticsKV(
// The statistics are ordered by their CreatedAt time (newest-to-oldest).
func (sc *TableStatisticsCache) GetTableStats(
ctx context.Context, table catalog.TableDescriptor,
) ([]*TableStatistic, error) {
) (stats []*TableStatistic, err error) {
if !statsUsageAllowed(table, sc.settings) {
return nil, nil
}
defer func() {
if r := recover(); r != nil {
// In the event of a "safe" panic, we only want to log the error and
// continue executing the query without stats for this table.
if ok, e := errorutil.ShouldCatch(r); ok {
err = e
} else {
// Other panic objects can't be considered "safe" and thus are
// propagated as crashes that terminate the session.
panic(r)
}
}
}()
forecast := forecastAllowed(table, sc.settings)
return sc.getTableStatsFromCache(ctx, table.GetID(), &forecast)
}
Expand Down

0 comments on commit 5d0e071

Please sign in to comment.