Merge #67106 #70280 #70281 #70293

67106: importccl: write stub table statistics during import r=rytaft,dt a=michae2

Addresses: #36273

At the end of an import job we notify the StatsRefresher that it should
collect statistics for the tables we imported into, but it can take a
while for those statistics to actually be collected. Meanwhile, if the
table is new, the optimizer will be operating totally blind. To make
this period slightly less painful, write stub statistics at the end of
import consisting of only row counts.

Release note (performance improvement): collect basic table statistics
during import, to help the optimizer until full statistics collection
completes.

70280: roachtest: harden the sqlsmith test r=yuzefovich a=yuzefovich

Previously, we had some false positives from the `sqlsmith` roachtest
filed because of "inbox communication errors" which were actually
triggered because of the vectorized panic injection. These errors
usually mean that a node died, so we used the errors as a proxy for the
crash. This commit adjusts the test to instead ping all nodes in the
cluster to see whether they are up or not and not rely on the
communication errors. This allows us to ignore the false positives
because of the panic injection.

Fixes: #66174.

Release note: None

70281: sql: fix JSON deserialization for sql stats lastExecAt field r=maryliag,xinhaoz a=Azhng

Previsouly, lastExecAt field for roachpb.CollectedStatementStatistics
was not properly updated. This caused the status API to return
empty data for that field.
This commit fixes the deserialization and extended the randomize
testing framework to also test time.Time type.

Partially Resolves #69675

Release Justification: Bug fixes and low-risk updates to new
functionality

Release note (bug fix): Last Execution Timestamp is now properly
updating.

70293: roachtest: unskip `rapid-restart` r=bananabrick a=cameronnunez

Informs [#63795]( #63795).

The rapid restart roachtest is getting unskipped in order to investigate CI failures.

Release note: None

Co-authored-by: Michael Erickson <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Azhng <[email protected]>
Co-authored-by: Cameron Nunez <[email protected]>

pkg/ccl/importccl/BUILD.bazel

@@ -55,6 +55,7 @@ go_library(
         "//pkg/sql/faketreeeval",
         "//pkg/sql/gcjob",
         "//pkg/sql/lexbase",
+        "//pkg/sql/opt/memo",
         "//pkg/sql/parser",
         "//pkg/sql/pgwire/pgcode",
         "//pkg/sql/pgwire/pgerror",
@@ -67,6 +68,7 @@ go_library(
         "//pkg/sql/sem/tree",
         "//pkg/sql/sessiondata",
         "//pkg/sql/sqltelemetry",
+        "//pkg/sql/stats",
         "//pkg/sql/types",
         "//pkg/util",
         "//pkg/util/bufalloc",

pkg/ccl/importccl/import_stmt.go

@@ -49,12 +49,14 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/gcjob"
+	"github.com/cockroachdb/cockroach/pkg/sql/opt/memo"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgnotice"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqltelemetry"
+	"github.com/cockroachdb/cockroach/pkg/sql/stats"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -2155,7 +2157,7 @@ func (r *importResumer) Resume(ctx context.Context, execCtx interface{}) error {
 		return err
 	}
 
-	if err := r.publishTables(ctx, p.ExecCfg()); err != nil {
+	if err := r.publishTables(ctx, p.ExecCfg(), res); err != nil {
 		return err
 	}
 
@@ -2342,7 +2344,9 @@ func (r *importResumer) checkForUDTModification(
 }
 
 // publishTables updates the status of imported tables from OFFLINE to PUBLIC.
-func (r *importResumer) publishTables(ctx context.Context, execCfg *sql.ExecutorConfig) error {
+func (r *importResumer) publishTables(
+	ctx context.Context, execCfg *sql.ExecutorConfig, res roachpb.BulkOpSummary,
+) error {
 	details := r.job.Details().(jobspb.ImportDetails)
 	// Tables should only be published once.
 	if details.TablesPublished {
@@ -2394,6 +2398,39 @@ func (r *importResumer) publishTables(ctx context.Context, execCfg *sql.Executor
 			return errors.Wrap(err, "publishing tables")
 		}
 
+		// Write "stub" statistics for new tables, which should be good enough to use
+		// until the full CREATE STATISTICS run finishes.
+		for _, tbl := range details.Tables {
+			if tbl.IsNew {
+				desc := tabledesc.NewUnsafeImmutable(tbl.Desc)
+				id := roachpb.BulkOpSummaryID(uint64(desc.GetID()), uint64(desc.GetPrimaryIndexID()))
+				rowCount := uint64(res.EntryCounts[id])
+				// TODO(michae2): collect distinct and null counts during import.
+				distinctCount := uint64(float64(rowCount) * memo.UnknownDistinctCountRatio)
+				nullCount := uint64(float64(rowCount) * memo.UnknownNullCountRatio)
+				// Because we don't yet have real distinct and null counts, only produce
+				// single-column stats to avoid the appearance of perfectly correlated
+				// columns.
+				multiColEnabled := false
+				statistics, err := sql.StubTableStats(desc, jobspb.ImportStatsName, multiColEnabled)
+				if err == nil {
+					for _, statistic := range statistics {
+						statistic.RowCount = rowCount
+						statistic.DistinctCount = distinctCount
+						statistic.NullCount = nullCount
+					}
+					err = stats.InsertNewStats(ctx, execCfg.InternalExecutor, txn, statistics)
+				}
+				if err != nil {
+					// Failure to create statistics should not fail the entire import.
+					log.Warningf(
+						ctx, "error while creating statistics during import of %q: %v",
+						desc.GetName(), err,
+					)
+				}
+			}
+		}
+
 		// Update job record to mark tables published state as complete.
 		details.TablesPublished = true
 		err := r.job.SetDetails(ctx, txn, details)

pkg/ccl/importccl/import_stmt_test.go

@@ -1113,6 +1113,54 @@ CREATE TABLE t (a duration);
 			typ:    "CSV",
 			err:    `"s" not found`,
 		},
+		{
+			name:   "statistics collection",
+			create: "a INT",
+			typ:    "CSV",
+			data: "0\n1\n2\n3\n4\n5\n6\n7\n8\n9\n" +
+				"10\n11\n12\n13\n14\n15\n16\n17\n18\n19\n" +
+				"20\n21\n22\n23\n24\n25\n26\n27\n28\n29\n" +
+				"30\n31\n32\n33\n34\n35\n36\n37\n38\n39\n" +
+				"40\n41\n42\n43\n44\n45\n46\n47\n48\n49\n" +
+				"50\n51\n52\n53\n54\n55\n56\n57\n58\n59\n" +
+				"60\n61\n62\n63\n64\n65\n66\n67\n68\n69\n" +
+				"70\n71\n72\n73\n74\n75\n76\n77\n78\n79\n" +
+				"80\n81\n82\n83\n84\n85\n86\n87\n88\n89\n" +
+				"90\n91\n92\n93\n94\n95\n96\n97\n98\n99\n",
+			query: map[string][][]string{
+				"SELECT column_names, row_count, distinct_count, null_count " +
+					"FROM [SHOW STATISTICS FOR TABLE t] " +
+					"WHERE statistics_name = '__import__' " +
+					"ORDER BY column_names": {
+					{"{a}", "100", "10", "1"},
+					{"{rowid}", "100", "10", "1"},
+				},
+			},
+		},
+		{
+			name:   "statistics collection multi",
+			create: "a INT PRIMARY KEY, b INT, INDEX (b, a)",
+			typ:    "CSV",
+			data: "0,0\n1,1\n2,2\n3,3\n4,4\n5,5\n6,6\n7,7\n8,8\n9,9\n" +
+				"10,10\n11,11\n12,12\n13,13\n14,14\n15,15\n16,16\n17,17\n18,18\n19,19\n" +
+				"20,20\n21,21\n22,22\n23,23\n24,24\n25,25\n26,26\n27,27\n28,28\n29,29\n" +
+				"30,30\n31,31\n32,32\n33,33\n34,34\n35,35\n36,36\n37,37\n38,38\n39,39\n" +
+				"40,40\n41,41\n42,42\n43,43\n44,44\n45,45\n46,46\n47,47\n48,48\n49,49\n" +
+				"50,50\n51,51\n52,52\n53,53\n54,54\n55,55\n56,56\n57,57\n58,58\n59,59\n" +
+				"60,60\n61,61\n62,62\n63,63\n64,64\n65,65\n66,66\n67,67\n68,68\n69,69\n" +
+				"70,70\n71,71\n72,72\n73,73\n74,74\n75,75\n76,76\n77,77\n78,78\n79,79\n" +
+				"80,80\n81,81\n82,82\n83,83\n84,84\n85,85\n86,86\n87,87\n88,88\n89,89\n" +
+				"90,90\n91,91\n92,92\n93,93\n94,94\n95,95\n96,96\n97,97\n98,98\n99,99",
+			query: map[string][][]string{
+				"SELECT column_names, row_count, distinct_count, null_count " +
+					"FROM [SHOW STATISTICS FOR TABLE t] " +
+					"WHERE statistics_name = '__import__' " +
+					"ORDER BY column_names": {
+					{"{a}", "100", "10", "1"},
+					{"{b}", "100", "10", "1"},
+				},
+			},
+		},
 	}
 
 	var mockRecorder struct {

pkg/cmd/roachtest/tests/acceptance.go

@@ -67,10 +67,7 @@ func registerAcceptance(r registry.Registry) {
 			{name: "build-analyze", fn: RunBuildAnalyze},
 			{name: "cli/node-status", fn: runCLINodeStatus},
 			{name: "cluster-init", fn: runClusterInit},
-			{
-				name: "rapid-restart", fn: runRapidRestart,
-				skip: "https://github.com/cockroachdb/cockroach/issues/63795",
-			},
+			{name: "rapid-restart", fn: runRapidRestart},
 			{name: "status-server", fn: runStatusServer},
 		},
 	}

pkg/cmd/roachtest/tests/sqlsmith.go

@@ -12,6 +12,7 @@ package tests
 
 import (
 	"context"
+	gosql "database/sql"
 	"fmt"
 	"math/rand"
 	"os"
@@ -28,6 +29,7 @@ import (
 )
 
 func registerSQLSmith(r registry.Registry) {
+	const numNodes = 4
 	setups := map[string]sqlsmith.Setup{
 		"empty":       sqlsmith.Setups["empty"],
 		"seed":        sqlsmith.Setups["seed"],
@@ -102,7 +104,11 @@ func registerSQLSmith(r registry.Registry) {
 		setup := setupFunc(rng)
 		setting := settingFunc(rng)
 
-		conn := c.Conn(ctx, 1)
+		allConns := make([]*gosql.DB, 0, numNodes)
+		for node := 1; node <= numNodes; node++ {
+			allConns = append(allConns, c.Conn(ctx, node))
+		}
+		conn := allConns[0]
 		t.Status("executing setup")
 		t.L().Printf("setup:\n%s", setup)
 		if _, err := conn.Exec(setup); err != nil {
@@ -132,17 +138,11 @@ func registerSQLSmith(r registry.Registry) {
 		// other setup queries have already completed, including the smither
 		// instantiation (otherwise, the setup might fail because of the
 		// injected panics).
-		if rng.Float64() < 0.5 {
-			// TODO(yuzefovich): at the moment we're only injecting panics with
-			// 50% probability in order to test the hypothesis that this panic
-			// injection is the root cause of the inbox communication errors we
-			// have been seeing sporadically.
-			injectPanicsStmt := "SET testing_vectorize_inject_panics=true;"
-			if _, err := conn.Exec(injectPanicsStmt); err != nil {
-				t.Fatal(err)
-			}
-			logStmt(injectPanicsStmt)
+		injectPanicsStmt := "SET testing_vectorize_inject_panics=true;"
+		if _, err := conn.Exec(injectPanicsStmt); err != nil {
+			t.Fatal(err)
 		}
+		logStmt(injectPanicsStmt)
 
 		t.Status("smithing")
 		until := time.After(t.Spec().(*registry.TestSpec).Timeout / 2)
@@ -222,11 +222,6 @@ func registerSQLSmith(r registry.Registry) {
 						logStmt(stmt)
 						t.Fatalf("error: %s\nstmt:\n%s;", err, stmt)
 					}
-				} else if strings.Contains(es, "communication error") {
-					// A communication error can be because
-					// a non-gateway node has crashed.
-					logStmt(stmt)
-					t.Fatalf("error: %s\nstmt:\n%s;", err, stmt)
 				} else if strings.Contains(es, "Empty statement returned by generate") ||
 					stmt == "" {
 					// Either were unable to generate a statement or
@@ -237,12 +232,15 @@ func registerSQLSmith(r registry.Registry) {
 				// frequently (due to sqlsmith not crafting
 				// executable queries 100% of the time) and are
 				// never interesting.
+				// TODO(yuzefovich): reevaluate this assumption.
 			}
 
-			// Ping the gateway to make sure it didn't crash.
-			if err := conn.PingContext(ctx); err != nil {
-				logStmt(stmt)
-				t.Fatalf("ping: %v\nprevious sql:\n%s;", err, stmt)
+			// Ping all nodes to make sure they didn't crash.
+			for idx, c := range allConns {
+				if err := c.PingContext(ctx); err != nil {
+					logStmt(stmt)
+					t.Fatalf("ping node %d: %v\nprevious sql:\n%s;", idx+1, err, stmt)
+				}
 			}
 		}
 	}
@@ -252,7 +250,7 @@ func registerSQLSmith(r registry.Registry) {
 			Name: fmt.Sprintf("sqlsmith/setup=%s/setting=%s", setup, setting),
 			// NB: sqlsmith failures should never block a release.
 			Owner:   registry.OwnerSQLQueries,
-			Cluster: r.MakeClusterSpec(4),
+			Cluster: r.MakeClusterSpec(numNodes),
 			Timeout: time.Minute * 20,
 			Run: func(ctx context.Context, t test.Test, c cluster.Cluster) {
 				runSQLSmith(ctx, t, c, setup, setting)

pkg/jobs/jobspb/wrap.go

@@ -71,6 +71,10 @@ var _ base.SQLInstanceID
 // running CREATE STATISTICS manually.
 const AutoStatsName = "__auto__"
 
+// ImportStatsName is the name to use for statistics created automatically
+// during import.
+const ImportStatsName = "__import__"
+
 // AutomaticJobTypes is a list of automatic job types that currently exist.
 var AutomaticJobTypes = [...]Type{
 	TypeAutoCreateStats,

pkg/sql/colflow/colrpc/inbox.go

@@ -323,7 +323,7 @@ func (i *Inbox) Next() coldata.Batch {
 			}
 			// Note that here err can be stream's context cancellation.
 			// Regardless of the cause we want to propagate such an error as
-			// expected on in all cases so that the caller could decide on how
+			// expected one in all cases so that the caller could decide on how
 			// to handle it.
 			err = pgerror.Newf(pgcode.InternalConnectionFailure, "inbox communication error: %s", err)
 			i.errCh <- err

pkg/sql/create_stats.go

@@ -62,6 +62,26 @@ var featureStatsEnabled = settings.RegisterBoolSetting(
 const defaultHistogramBuckets = 200
 const nonIndexColHistogramBuckets = 2
 
+// StubTableStats generates "stub" statistics for a table which are missing
+// histograms and have 0 for all values.
+func StubTableStats(
+	desc catalog.TableDescriptor, name string, multiColEnabled bool,
+) ([]*stats.TableStatisticProto, error) {
+	colStats, err := createStatsDefaultColumns(desc, multiColEnabled)
+	if err != nil {
+		return nil, err
+	}
+	statistics := make([]*stats.TableStatisticProto, len(colStats))
+	for i, colStat := range colStats {
+		statistics[i] = &stats.TableStatisticProto{
+			TableID:   desc.GetID(),
+			Name:      name,
+			ColumnIDs: colStat.ColumnIDs,
+		}
+	}
+	return statistics, nil
+}
+
 // createStatsNode is a planNode implemented in terms of a function. The
 // startJob function starts a Job during Start, and the remainder of the
 // CREATE STATISTICS planning and execution is performed within the jobs

pkg/sql/opt/memo/statistics_builder.go

@@ -445,7 +445,7 @@ func (sb *statisticsBuilder) colStatLeaf(
 			if nullableCols.Equals(colSet) {
 				// No column statistics on this colSet - use the unknown
 				// null count ratio.
-				colStat.NullCount = s.RowCount * unknownNullCountRatio
+				colStat.NullCount = s.RowCount * UnknownNullCountRatio
 			} else {
 				colStatLeaf := sb.colStatLeaf(nullableCols, s, fd, notNullCols)
 				// Fetch the colStat again since it may now have a different address.
@@ -460,8 +460,8 @@ func (sb *statisticsBuilder) colStatLeaf(
 
 	if colSet.Len() == 1 {
 		col, _ := colSet.Next(0)
-		colStat.DistinctCount = unknownDistinctCountRatio * s.RowCount
-		colStat.NullCount = unknownNullCountRatio * s.RowCount
+		colStat.DistinctCount = UnknownDistinctCountRatio * s.RowCount
+		colStat.NullCount = UnknownNullCountRatio * s.RowCount
 		if notNullCols.Contains(col) {
 			colStat.NullCount = 0
 		}
@@ -2121,7 +2121,7 @@ func (sb *statisticsBuilder) colStatMax1Row(
 	s := &max1Row.Relational().Stats
 	colStat, _ := s.ColStats.Add(colSet)
 	colStat.DistinctCount = 1
-	colStat.NullCount = s.RowCount * unknownNullCountRatio
+	colStat.NullCount = s.RowCount * UnknownNullCountRatio
 	if colSet.Intersects(max1Row.Relational().NotNullCols) {
 		colStat.NullCount = 0
 	}
@@ -2319,7 +2319,7 @@ func (sb *statisticsBuilder) colStatProjectSet(
 					// requested columns correspond to, and estimate the distinct count and
 					// null count based on the type of generator function and its parameters.
 					zipColsDistinctCount *= unknownGeneratorRowCount * unknownGeneratorDistinctCountRatio
-					zipColsNullCount *= unknownNullCountRatio
+					zipColsNullCount *= UnknownNullCountRatio
 				} else {
 					// The columns(s) contain a scalar function or expression.
 					// These columns can contain many null values if the zip also
@@ -2346,7 +2346,7 @@ func (sb *statisticsBuilder) colStatProjectSet(
 				if item.ScalarProps().OuterCols.Intersects(inputProps.OutputCols) {
 					// The column(s) are correlated with the input, so they may have a
 					// distinct value for each distinct row of the input.
-					zipColsDistinctCount *= inputStats.RowCount * unknownDistinctCountRatio
+					zipColsDistinctCount *= inputStats.RowCount * UnknownDistinctCountRatio
 				}
 			}
 		}
@@ -2788,14 +2788,15 @@ const (
 	// This is an arbitrary row count used in the absence of any real statistics.
 	unknownRowCount = 1000
 
-	// This is the ratio of distinct column values to number of rows, which is
-	// used in the absence of any real statistics for non-key columns.
-	// TODO(rytaft): See if there is an industry standard value for this.
-	unknownDistinctCountRatio = 0.1
+	// UnknownDistinctCountRatio is the ratio of distinct column values to number
+	// of rows, which is used in the absence of any real statistics for non-key
+	// columns.  TODO(rytaft): See if there is an industry standard value for
+	// this.
+	UnknownDistinctCountRatio = 0.1
 
-	// This is the ratio of null column values to number of rows for nullable
-	// columns, which is used in the absence of any real statistics.
-	unknownNullCountRatio = 0.01
+	// UnknownNullCountRatio is the ratio of null column values to number of rows
+	// for nullable columns, which is used in the absence of any real statistics.
+	UnknownNullCountRatio = 0.01
 
 	// Use a small row count for generator functions; this allows use of lookup
 	// join in cases like using json_array_elements with a small constant array.
@@ -3198,7 +3199,7 @@ func (sb *statisticsBuilder) applyIndexConstraint(
 		numConjuncts := sb.numConjunctsInConstraint(c, i)
 
 		// Set the distinct count for the current column of the constraint
-		// according to unknownDistinctCountRatio.
+		// according to UnknownDistinctCountRatio.
 		var lowerBound float64
 		if i == applied {
 			lowerBound = lastColMinDistinct
@@ -3250,7 +3251,7 @@ func (sb *statisticsBuilder) applyConstraintSet(
 			numConjuncts := sb.numConjunctsInConstraint(c, 0 /* nth */)
 
 			// Set the distinct count for the first column of the constraint
-			// according to unknownDistinctCountRatio.
+			// according to UnknownDistinctCountRatio.
 			sb.updateDistinctCountFromUnappliedConjuncts(col, e, s, numConjuncts, lastColMinDistinct)
 		}
 

pkg/sql/sqlstats/persistedsqlstats/sqlstatsutil/BUILD.bazel

@@ -16,6 +16,7 @@ go_library(
         "//pkg/sql/sem/tree",
         "//pkg/util/encoding",
         "//pkg/util/json",
+        "//pkg/util/timeutil",
         "@com_github_cockroachdb_apd_v2//:apd",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_stretchr_testify//require",