Merge cockroachdb#56395 cockroachdb#57222

56395: sqlsmith: add schema-related operations r=jordanlewis a=jordanlewis User-defined schemas are now supported in sqlsmith. UDSs will be randomly generated, and new tables will be randomly included in the available UDSs. Queries will select from any table, including those inside of UDSs as well. Closes cockroachdb#54961. Release note: None 57222: Revert "Revert "util/log: more misc cleanups"" r=irfansharif a=knz Reverts cockroachdb#57178. This re-instates cockroachdb#57000, as it did not "introduce a race in crdb" The description in cockroachdb#57178 was incorrect - instead cockroachdb#57161 / cockroachdb#57162 merely outlined a bug in some tests, which remains to be fixed. This PR also includes a temporary workaround for said test bug. Fixes cockroachdb#57162. Fixes cockroachdb#57161 (presumably - although the symptoms there don't align). Co-authored-by: Jordan Lewis <[email protected]> Co-authored-by: Raphael 'kena' Poss <[email protected]>
otan-cockroach · Nov 30, 2020 · 13ce0e8 · 13ce0e8
3 parents b7de6c9 + db48bd5 + ef184b7
commit 13ce0e8
Show file tree

Hide file tree

Showing 62 changed files with 520 additions and 416 deletions.
diff --git a/pkg/ccl/changefeedccl/BUILD.bazel b/pkg/ccl/changefeedccl/BUILD.bazel
@@ -75,6 +75,7 @@ go_library(
         "//pkg/util/humanizeutil",
         "//pkg/util/json",
         "//pkg/util/log",
+        "//pkg/util/log/logcrash",
         "//pkg/util/metric",
         "//pkg/util/mon",
         "//pkg/util/protoutil",

diff --git a/pkg/ccl/changefeedccl/changefeed_processors.go b/pkg/ccl/changefeedccl/changefeed_processors.go
@@ -31,6 +31,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/span"
@@ -715,7 +716,7 @@ func (cf *changeFrontier) noteResolvedSpan(d rowenc.EncDatum) error {
 	// job progress update closure, but it currently doesn't pass along the info
 	// we'd need to do it that way.
 	if !resolved.Timestamp.IsEmpty() && resolved.Timestamp.Less(cf.highWaterAtStart) {
-		log.ReportOrPanic(cf.Ctx, &cf.flowCtx.Cfg.Settings.SV,
+		logcrash.ReportOrPanic(cf.Ctx, &cf.flowCtx.Cfg.Settings.SV,
 			`got a span level timestamp %s for %s that is less than the initial high-water %s`,
 			log.Safe(resolved.Timestamp), resolved.Span, log.Safe(cf.highWaterAtStart))
 		return nil

diff --git a/pkg/cli/BUILD.bazel b/pkg/cli/BUILD.bazel
@@ -144,6 +144,7 @@ go_library(
         "//pkg/util/iterutil",
         "//pkg/util/keysutil",
         "//pkg/util/log",
+        "//pkg/util/log/logcrash",
         "//pkg/util/log/logflags",
         "//pkg/util/log/logpb",
         "//pkg/util/log/severity",

diff --git a/pkg/cli/cli.go b/pkg/cli/cli.go
@@ -23,6 +23,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logflags"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	// intentionally not all the workloads in pkg/ccl/workloadccl/allccl
@@ -58,12 +59,12 @@ func Main() {
 
 	cmdName := commandName(os.Args[1:])
 
-	log.SetupCrashReporter(
+	logcrash.SetupCrashReporter(
 		context.Background(),
 		cmdName,
 	)
 
-	defer log.RecoverAndReportPanic(context.Background(), &serverCfg.Settings.SV)
+	defer logcrash.RecoverAndReportPanic(context.Background(), &serverCfg.Settings.SV)
 
 	err := Run(os.Args[1:])
 

diff --git a/pkg/cli/cpuprofile.go b/pkg/cli/cpuprofile.go
@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 )
 
@@ -76,7 +77,7 @@ func initCPUProfile(ctx context.Context, dir string, st *cluster.Settings) {
 	// TODO(knz,tbg): The caller of initCPUProfile() also defines a stopper;
 	// arguably this code would be better served by stopper.RunAsyncTask().
 	go func() {
-		defer log.RecoverAndReportPanic(ctx, &serverCfg.Settings.SV)
+		defer logcrash.RecoverAndReportPanic(ctx, &serverCfg.Settings.SV)
 
 		ctx := context.Background()
 

diff --git a/pkg/cli/start.go b/pkg/cli/start.go
@@ -46,6 +46,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/humanizeutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/log/logflags"
 	"github.com/cockroachdb/cockroach/pkg/util/log/severity"
 	"github.com/cockroachdb/cockroach/pkg/util/sdnotify"
@@ -526,7 +527,7 @@ If problems persist, please see %s.`
 				// actually been started successfully. If there's no server,
 				// we won't know enough to decide whether reporting is
 				// permitted.
-				log.RecoverAndReportPanic(ctx, &s.ClusterSettings().SV)
+				logcrash.RecoverAndReportPanic(ctx, &s.ClusterSettings().SV)
 			}
 		}()
 		// When the start up goroutine completes, so can the start up span

diff --git a/pkg/internal/sqlsmith/alter.go b/pkg/internal/sqlsmith/alter.go
@@ -21,6 +21,7 @@ var (
 	alters               = append(append(altersTableExistence, altersExistingTable...), altersTypeExistence...)
 	altersTableExistence = []statementWeight{
 		{10, makeCreateTable},
+		{2, makeCreateSchema},
 		{1, makeDropTable},
 	}
 	altersExistingTable = []statementWeight{
@@ -62,9 +63,20 @@ func makeAlter(s *Smither) (tree.Statement, bool) {
 	return nil, false
 }
 
+func makeCreateSchema(s *Smither) (tree.Statement, bool) {
+	return &tree.CreateSchema{
+		Schema: tree.ObjectNamePrefix{
+			SchemaName:     s.name("schema"),
+			ExplicitSchema: true,
+		},
+	}, true
+}
+
 func makeCreateTable(s *Smither) (tree.Statement, bool) {
 	table := rowenc.RandCreateTable(s.rnd, "", 0)
-	table.Table = tree.MakeUnqualifiedTableName(s.name("tab"))
+	schemaOrd := s.rnd.Intn(len(s.schemas))
+	schema := s.schemas[schemaOrd]
+	table.Table = tree.MakeTableNameWithSchema(tree.Name(s.dbName), schema.SchemaName, s.name("tab"))
 	return table, true
 }
 

diff --git a/pkg/internal/sqlsmith/schema.go b/pkg/internal/sqlsmith/schema.go
@@ -55,6 +55,10 @@ func (s *Smither) ReloadSchemas() error {
 	if err != nil {
 		return err
 	}
+	s.schemas, err = s.extractSchemas()
+	if err != nil {
+		return err
+	}
 	s.indexes, err = s.extractIndexes(s.tables)
 	s.columns = make(map[tree.TableName]map[tree.Name]*tree.ColumnTableDef)
 	for _, ref := range s.tables {
@@ -208,6 +212,31 @@ FROM
 	}, nil
 }
 
+type schemaRef struct {
+	SchemaName tree.Name
+}
+
+func (s *Smither) extractSchemas() ([]*schemaRef, error) {
+	rows, err := s.db.Query(`
+SELECT nspname FROM pg_catalog.pg_namespace
+WHERE nspname NOT IN ('crdb_internal', 'pg_catalog', 'pg_extension',
+		'information_schema')`)
+	if err != nil {
+		return nil, err
+	}
+	defer rows.Close()
+
+	var ret []*schemaRef
+	for rows.Next() {
+		var schema tree.Name
+		if err := rows.Scan(&schema); err != nil {
+			return nil, err
+		}
+		ret = append(ret, &schemaRef{SchemaName: schema})
+	}
+	return ret, nil
+}
+
 func (s *Smither) extractTables() ([]*tableRef, error) {
 	rows, err := s.db.Query(`
 SELECT
@@ -222,7 +251,8 @@ SELECT
 FROM
 	information_schema.columns
 WHERE
-	table_schema = 'public'
+	table_schema NOT IN ('crdb_internal', 'pg_catalog', 'pg_extension',
+	                     'information_schema')
 ORDER BY
 	table_catalog, table_schema, table_name
 	`)
@@ -241,9 +271,6 @@ ORDER BY
 	var tables []*tableRef
 	var currentCols []*tree.ColumnTableDef
 	emit := func() error {
-		if lastSchema != "public" {
-			return nil
-		}
 		if len(currentCols) == 0 {
 			return fmt.Errorf("zero columns for %s.%s", lastCatalog, lastName)
 		}
@@ -255,8 +282,9 @@ ORDER BY
 				Type: col.Type,
 			})
 		}
+		tableName := tree.MakeTableNameWithSchema(lastCatalog, lastSchema, lastName)
 		tables = append(tables, &tableRef{
-			TableName: tree.NewTableName(lastCatalog, lastName),
+			TableName: &tableName,
 			Columns:   currentCols,
 		})
 		return nil
@@ -324,9 +352,13 @@ func (s *Smither) extractIndexes(
 		// sqlsmith.
 		rows, err := s.db.Query(fmt.Sprintf(`
 			SELECT
-			    index_name, column_name, storing, direction = 'ASC'
+			    si.index_name, column_name, storing, direction = 'ASC',
+          is_inverted
 			FROM
-			    [SHOW INDEXES FROM %s]
+			    [SHOW INDEXES FROM %s] si
+      JOIN crdb_internal.table_indexes ti
+           ON si.table_name = ti.descriptor_name
+           AND si.index_name = ti.index_name
 			WHERE
 			    column_name != 'rowid'
 			`, t.TableName))
@@ -335,15 +367,16 @@ func (s *Smither) extractIndexes(
 		}
 		for rows.Next() {
 			var idx, col tree.Name
-			var storing, ascending bool
-			if err := rows.Scan(&idx, &col, &storing, &ascending); err != nil {
+			var storing, ascending, inverted bool
+			if err := rows.Scan(&idx, &col, &storing, &ascending, &inverted); err != nil {
 				rows.Close()
 				return nil, err
 			}
 			if _, ok := indexes[idx]; !ok {
 				indexes[idx] = &tree.CreateIndex{
-					Name:  idx,
-					Table: *t.TableName,
+					Name:     idx,
+					Table:    *t.TableName,
+					Inverted: inverted,
 				}
 			}
 			create := indexes[idx]
@@ -359,25 +392,10 @@ func (s *Smither) extractIndexes(
 					Direction: dir,
 				})
 			}
-			row := s.db.QueryRow(fmt.Sprintf(`
-			SELECT
-			    is_inverted
-			FROM
-			    crdb_internal.table_indexes
-			WHERE
-			    descriptor_name = '%s' AND index_name = '%s'
-`, t.TableName.Table(), idx))
-			var isInverted bool
-			if err = row.Scan(&isInverted); err != nil {
-				// We got an error which likely indicates that 'is_inverted' column is
-				// not present in crdb_internal.table_indexes vtable (probably because
-				// we're running 19.2 version). We will use a heuristic to determine
-				// whether the index is inverted.
-				isInverted = strings.Contains(strings.ToLower(idx.String()), "jsonb")
-			}
-			indexes[idx].Inverted = isInverted
 		}
-		rows.Close()
+		if err := rows.Close(); err != nil {
+			return nil, err
+		}
 		if err := rows.Err(); err != nil {
 			return nil, err
 		}

diff --git a/pkg/internal/sqlsmith/sqlsmith.go b/pkg/internal/sqlsmith/sqlsmith.go
@@ -60,6 +60,8 @@ type Smither struct {
 	rnd              *rand.Rand
 	db               *gosql.DB
 	lock             syncutil.RWMutex
+	dbName           string
+	schemas          []*schemaRef
 	tables           []*tableRef
 	columns          map[tree.TableName]map[tree.Name]*tree.ColumnTableDef
 	indexes          map[tree.TableName]map[tree.Name]*tree.CreateIndex
@@ -128,6 +130,10 @@ func NewSmither(db *gosql.DB, rnd *rand.Rand, opts ...SmitherOption) (*Smither,
 	s.scalarExprSampler = newWeightedScalarExprSampler(s.scalarExprWeights, rnd.Int63())
 	s.boolExprSampler = newWeightedScalarExprSampler(s.boolExprWeights, rnd.Int63())
 	s.enableBulkIO()
+	row := s.db.QueryRow("SELECT current_database()")
+	if err := row.Scan(&s.dbName); err != nil {
+		return nil, err
+	}
 	return s, s.ReloadSchemas()
 }
 

diff --git a/pkg/jobs/BUILD.bazel b/pkg/jobs/BUILD.bazel
@@ -46,6 +46,7 @@ go_library(
         "//pkg/util/envutil",
         "//pkg/util/hlc",
         "//pkg/util/log",
+        "//pkg/util/log/logcrash",
         "//pkg/util/metric",
         "//pkg/util/protoutil",
         "//pkg/util/retry",

diff --git a/pkg/jobs/adopt.go b/pkg/jobs/adopt.go
@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlliveness"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/tracing"
 	"github.com/cockroachdb/errors"
 )
@@ -259,7 +260,7 @@ func (r *Registry) runJob(
 		// assertion errors, which shouldn't cause the test to panic. For now,
 		// comment this out.
 		// if errors.HasAssertionFailure(err) {
-		// 	log.ReportOrPanic(ctx, nil, err.Error())
+		// 	logcrash.ReportOrPanic(ctx, nil, err.Error())
 		// }
 		log.Errorf(ctx, "job %d: adoption completed with error %v", *job.ID(), err)
 	}
@@ -308,7 +309,7 @@ RETURNING id, status`,
 				log.Infof(ctx, "job %d, session id: %s canceled: the job is now reverting",
 					id, s.ID())
 			default:
-				log.ReportOrPanic(ctx, nil, "unexpected job status")
+				logcrash.ReportOrPanic(ctx, nil, "unexpected job status")
 			}
 		}
 		return nil

diff --git a/pkg/jobs/deprecated.go b/pkg/jobs/deprecated.go
@@ -375,7 +375,7 @@ func (r *Registry) deprecatedResume(
 				// assertion errors, which shouldn't cause the test to panic. For now,
 				// comment this out.
 				// if errors.HasAssertionFailure(err) {
-				// 	log.ReportOrPanic(ctx, nil, err.Error())
+				// 	logcrash.ReportOrPanic(ctx, nil, err.Error())
 				// }
 				log.Errorf(ctx, "job %d: adoption completed with error %v", *job.ID(), err)
 			}

diff --git a/pkg/server/BUILD.bazel b/pkg/server/BUILD.bazel
@@ -135,6 +135,7 @@ go_library(
         "//pkg/util/httputil",
         "//pkg/util/humanizeutil",
         "//pkg/util/log",
+        "//pkg/util/log/logcrash",
         "//pkg/util/log/logpb",
         "//pkg/util/log/severity",
         "//pkg/util/metric",

diff --git a/pkg/server/admin.go b/pkg/server/admin.go
@@ -51,6 +51,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/contextutil"
 	"github.com/cockroachdb/cockroach/pkg/util/envutil"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/log/logcrash"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
@@ -1322,7 +1323,7 @@ func (s *adminServer) Cluster(
 
 	return &serverpb.ClusterResponse{
 		ClusterID:         clusterID.String(),
-		ReportingEnabled:  log.DiagnosticsReportingEnabled.Get(&s.server.st.SV),
+		ReportingEnabled:  logcrash.DiagnosticsReportingEnabled.Get(&s.server.st.SV),
 		EnterpriseEnabled: enterpriseEnabled,
 	}, nil
 }

diff --git a/pkg/server/debug/logspy.go b/pkg/server/debug/logspy.go
@@ -160,7 +160,7 @@ func (spy *logSpy) run(ctx context.Context, w io.Writer, opts logSpyOptions) (er
 		if err == nil {
 			if dropped := atomic.LoadInt32(&countDropped); dropped > 0 {
 				entry := log.MakeEntry(
-					ctx, severity.WARNING, nil /* LogCounter */, 0 /* depth */, false, /* redactable */
+					ctx, severity.WARNING, 0 /* depth */, false, /* redactable */
 					"%d messages were dropped", log.Safe(dropped))
 				err = log.FormatEntry(entry, w) // modify return value
 			}
@@ -175,7 +175,7 @@ func (spy *logSpy) run(ctx context.Context, w io.Writer, opts logSpyOptions) (er
 
 	{
 		entry := log.MakeEntry(
-			ctx, severity.INFO, nil /* LogCounter */, 0 /* depth */, false, /* redactable */
+			ctx, severity.INFO, 0 /* depth */, false, /* redactable */
 			"intercepting logs with options %+v", opts)
 		entries <- entry
 	}
@@ -212,8 +212,8 @@ func (spy *logSpy) run(ctx context.Context, w io.Writer, opts logSpyOptions) (er
 	for {
 		select {
 		case <-done:
-
 			return
+
 		case entry := <-entries:
 			if err := log.FormatEntry(entry, w); err != nil {
 				return errors.Wrapf(err, "while writing entry %v", entry)
@@ -225,6 +225,7 @@ func (spy *logSpy) run(ctx context.Context, w io.Writer, opts logSpyOptions) (er
 			if done == nil {
 				done = ctx.Done()
 			}
+
 		case <-flushTimer.C:
 			flushTimer.Read = true
 			flushTimer.Reset(flushInterval)