Merge pull request #149625 from cockroachdb/blathers/backport-release-25.2-149279

release-25.2: stats: improve handling of some expected errors

Author: yuzefovich

docs/generated/settings/settings-for-tenants.txt

@@ -369,7 +369,7 @@ sql.stats.multi_column_collection.enabled	boolean	true	multi-column statistics c
 sql.stats.non_default_columns.min_retention_period	duration	24h0m0s	minimum retention period for table statistics collected on non-default columns	application
 sql.stats.non_indexed_json_histograms.enabled	boolean	false	set to true to collect table statistics histograms on non-indexed JSON columns	application
 sql.stats.persisted_rows.max	integer	1000000	maximum number of rows of statement and transaction statistics that will be persisted in the system tables before compaction begins	application
-sql.stats.post_events.enabled	boolean	false	if set, an event is logged for every CREATE STATISTICS job	application
+sql.stats.post_events.enabled	boolean	false	if set, an event is logged for every successful CREATE STATISTICS job	application
 sql.stats.response.max	integer	20000	the maximum number of statements and transaction stats returned in a CombinedStatements request	application
 sql.stats.response.show_internal.enabled	boolean	false	controls if statistics for internal executions should be returned by the CombinedStatements and if internal sessions should be returned by the ListSessions endpoints. These endpoints are used to display statistics on the SQL Activity pages	application
 sql.stats.system_tables.enabled	boolean	true	when true, enables use of statistics on system tables by the query optimizer	application

docs/generated/settings/settings.html

@@ -324,7 +324,7 @@
 <tr><td><div id="setting-sql-stats-non-default-columns-min-retention-period" class="anchored"><code>sql.stats.non_default_columns.min_retention_period</code></div></td><td>duration</td><td><code>24h0m0s</code></td><td>minimum retention period for table statistics collected on non-default columns</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-stats-non-indexed-json-histograms-enabled" class="anchored"><code>sql.stats.non_indexed_json_histograms.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>set to true to collect table statistics histograms on non-indexed JSON columns</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-stats-persisted-rows-max" class="anchored"><code>sql.stats.persisted_rows.max</code></div></td><td>integer</td><td><code>1000000</code></td><td>maximum number of rows of statement and transaction statistics that will be persisted in the system tables before compaction begins</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
-<tr><td><div id="setting-sql-stats-post-events-enabled" class="anchored"><code>sql.stats.post_events.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if set, an event is logged for every CREATE STATISTICS job</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
+<tr><td><div id="setting-sql-stats-post-events-enabled" class="anchored"><code>sql.stats.post_events.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>if set, an event is logged for every successful CREATE STATISTICS job</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-stats-response-max" class="anchored"><code>sql.stats.response.max</code></div></td><td>integer</td><td><code>20000</code></td><td>the maximum number of statements and transaction stats returned in a CombinedStatements request</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-stats-response-show-internal-enabled" class="anchored"><code>sql.stats.response.show_internal.enabled</code></div></td><td>boolean</td><td><code>false</code></td><td>controls if statistics for internal executions should be returned by the CombinedStatements and if internal sessions should be returned by the ListSessions endpoints. These endpoints are used to display statistics on the SQL Activity pages</td><td>Serverless/Dedicated/Self-Hosted</td></tr>
 <tr><td><div id="setting-sql-stats-system-tables-enabled" class="anchored"><code>sql.stats.system_tables.enabled</code></div></td><td>boolean</td><td><code>true</code></td><td>when true, enables use of statistics on system tables by the query optimizer</td><td>Serverless/Dedicated/Self-Hosted</td></tr>

pkg/sql/create_stats.go

@@ -44,7 +44,7 @@ import (
 var createStatsPostEvents = settings.RegisterBoolSetting(
 	settings.ApplicationLevel,
 	"sql.stats.post_events.enabled",
-	"if set, an event is logged for every CREATE STATISTICS job",
+	"if set, an event is logged for every successful CREATE STATISTICS job",
 	false,
 	settings.WithPublic)
 
@@ -854,6 +854,8 @@ func (r *createStatsResumer) Resume(ctx context.Context, execCtx interface{}) er
 		return nil
 	}
 
+	// Note that we'll log an event even if the stats collection didn't occur
+	// due to a benign error.
 	// TODO(rytaft): This creates a new transaction for the CREATE STATISTICS
 	// event. It must be different from the CREATE STATISTICS transaction,
 	// because that transaction must be read-only. In the future we may want

pkg/sql/distsql_physical_planner.go

@@ -4060,6 +4060,10 @@ func (dsp *DistSQLPlanner) createPhysPlanForPlanNode(
 				// Partial stats collections scan a different index for each column.
 				numIndexes = len(details.ColumnStats)
 			}
+			// If the error has pgcode.ObjectNotInPrerequisiteState, we don't
+			// swallow it here and propagate it to the client (since we're
+			// running EXPLAIN or EXPLAIN ANALYZE, the caller would be
+			// interested to know about all errors, benign included).
 			plan, err = dsp.createPlanForCreateStats(
 				ctx, planCtx, planCtx.planner.SemaCtx(), 0 /* jobID */, details,
 				numIndexes, 0, /* curIndex */

pkg/sql/distsql_plan_stats.go

@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
-	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/exec"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
@@ -349,10 +348,9 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 	)
 
 	var stat *stats.TableStatistic
-	var histogram []cat.HistogramBucket
-	// Find the statistic and histogram from the newest table statistic for our
-	// column that is not partial and not forecasted. The first one we find will
-	// be the latest due to the newest to oldest ordering property of the cache.
+	// Find the statistic from the newest table statistic for our column that is
+	// not partial and not forecasted. The first one we find will be the latest
+	// due to the newest to oldest ordering property of the cache.
 	for _, t := range tableStats {
 		if len(t.ColumnIDs) == 1 && column.GetID() == t.ColumnIDs[0] &&
 			!t.IsPartial() && !t.IsMerged() && !t.IsForecast() {
@@ -372,7 +370,6 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 				)
 			}
 			stat = t
-			histogram = t.Histogram
 			break
 		}
 	}
@@ -382,10 +379,18 @@ func (dsp *DistSQLPlanner) createPartialStatsPlan(
 			"column %s does not have a prior statistic",
 			column.GetName())
 	}
-	lowerBound, upperBound, err := bounds.GetUsingExtremesBounds(ctx, planCtx.EvalContext(), histogram)
+	lowerBound, upperBound, err := bounds.GetUsingExtremesBounds(ctx, planCtx.EvalContext(), stat.Histogram)
 	if err != nil {
 		return nil, err
 	}
+	if lowerBound == nil {
+		return nil, pgerror.Newf(
+			pgcode.ObjectNotInPrerequisiteState,
+			"only outer or NULL bounded buckets exist in %s@%s (table ID %d, column IDs %v), "+
+				"so partial stats cannot be collected",
+			scan.desc.GetName(), scan.index.GetName(), stat.TableID, stat.ColumnIDs,
+		)
+	}
 	extremesSpans, err := bounds.ConstructUsingExtremesSpans(lowerBound, upperBound, scan.index)
 	if err != nil {
 		return nil, err
@@ -724,6 +729,11 @@ func (dsp *DistSQLPlanner) createStatsPlan(
 		numIndexes, curIndex), nil
 }
 
+// createPlanForCreateStats creates the DistSQL plan to perform the table stats
+// collection according to CreateStatsDetails.
+//
+// If the returned error has pgcode.ObjectNotInPrerequisiteState, it might be
+// benign and the caller might want to swallow it, depending on the context.
 func (dsp *DistSQLPlanner) createPlanForCreateStats(
 	ctx context.Context,
 	planCtx *PlanningCtx,
@@ -781,6 +791,17 @@ func (dsp *DistSQLPlanner) planAndRunCreateStats(
 
 	physPlan, err := dsp.createPlanForCreateStats(ctx, planCtx, semaCtx, jobId, details, numIndexes, curIndex)
 	if err != nil {
+		if pgerror.GetPGCode(err) == pgcode.ObjectNotInPrerequisiteState {
+			// This error is benign, so we'll swallow it. Concretely, this means
+			// that we'll proceed with collecting partial stats if there are
+			// more to do and the stats job overall will be marked as having
+			// "succeeded". Even though this might seem confusing, it's a better
+			// trade-off than having auto partial stats fail repeatedly due to
+			// expected conditions (like a lower bound doesn't exist) raising
+			// concerns for users. See #149279 for more discussion.
+			log.Infof(ctx, "job %d: stats collection is swallowing benign error %v", jobId, err)
+			return nil
+		}
 		return err
 	}
 

pkg/sql/logictest/testdata/logic_test/distsql_stats

@@ -2702,10 +2702,10 @@ statement ok
 CREATE TABLE xyz (x INT, y INT, z INT, INDEX (x, y));
 
 statement error pq: column x does not have a prior statistic
-CREATE STATISTICS xyz_x ON x FROM xyz USING EXTREMES;
+EXPLAIN ANALYZE CREATE STATISTICS xyz_x ON x FROM xyz USING EXTREMES;
 
 statement error pq: the latest full statistic for column a has no histogram
-CREATE STATISTICS u_partial ON a FROM u USING EXTREMES;
+EXPLAIN ANALYZE CREATE STATISTICS u_partial ON a FROM u USING EXTREMES;
 
 statement error pq: table xy does not contain a non-partial forward index with y as a prefix column
 CREATE STATISTICS xy_y_partial ON y FROM xy USING EXTREMES;
@@ -2724,8 +2724,8 @@ CREATE STATISTICS only_null_stat ON a FROM only_null;
 statement ok
 SELECT crdb_internal.clear_table_stats_cache();
 
-statement error pq: only outer or NULL bounded buckets exist in the index, so partial stats cannot be collected
-CREATE STATISTICS only_null_partial ON a FROM only_null USING EXTREMES;
+statement error pq: only outer or NULL bounded buckets exist in only_null@only_null_a_idx \(table ID \d+, column IDs \[1\]\), so partial stats cannot be collected
+EXPLAIN ANALYZE CREATE STATISTICS only_null_partial ON a FROM only_null USING EXTREMES;
 
 statement ok
 CREATE INDEX ON xy (y) WHERE y > 5;

pkg/sql/stats/bounds/BUILD.bazel

@@ -10,8 +10,6 @@ go_library(
         "//pkg/sql/catalog/catenumpb",
         "//pkg/sql/opt/cat",
         "//pkg/sql/opt/constraint",
-        "//pkg/sql/pgwire/pgcode",
-        "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/sem/eval",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sem/tree/treecmp",

pkg/sql/stats/bounds/extremes.go

@@ -12,8 +12,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catenumpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/cat"
 	"github.com/cockroachdb/cockroach/pkg/sql/opt/constraint"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree/treecmp"
@@ -80,6 +78,9 @@ func ConstructUsingExtremesSpans(
 
 // GetUsingExtremesBounds returns a tree.Datum representing the exclusive upper
 // and exclusive lower bounds of the USING EXTREMES span for partial statistics.
+//
+// lowerBound can be nil which is the case when the exclusive lower bound
+// doesn't exist.
 func GetUsingExtremesBounds(
 	ctx context.Context, evalCtx *eval.Context, histogram []cat.HistogramBucket,
 ) (lowerBound tree.Datum, upperBound tree.Datum, _ error) {
@@ -95,8 +96,8 @@ func GetUsingExtremesBounds(
 		upperBound = histogram[len(histogram)-2].UpperBound
 	}
 
-	// Pick the earliest lowerBound that is not null and isn't an outer bucket,
-	// but if none exist, return error
+	// Pick the earliest lowerBound that is not null and isn't an outer bucket.
+	// We'll return nil if none exist which the caller is required to handle.
 	for i := range histogram {
 		hist := &histogram[i]
 		if cmp, err := hist.UpperBound.Compare(ctx, evalCtx, tree.DNull); err != nil {
@@ -106,11 +107,5 @@ func GetUsingExtremesBounds(
 			break
 		}
 	}
-	if lowerBound == nil {
-		return lowerBound, nil,
-			pgerror.Newf(
-				pgcode.ObjectNotInPrerequisiteState,
-				"only outer or NULL bounded buckets exist in the index, so partial stats cannot be collected")
-	}
 	return lowerBound, upperBound, nil
 }