sql: descriptor validation overhaul

pkg/base/test_server_args.go

@@ -128,9 +128,6 @@ type TestServerArgs struct {
 
 	// IF set, the demo login endpoint will be enabled.
 	EnableDemoLoginEndpoint bool
-
-	// If set, testing specific descriptor validation will be disabled. even if the server
-	DisableTestingDescriptorValidation bool
 }
 
 // TestClusterArgs contains the parameters one can set when creating a test

pkg/ccl/backupccl/backupbase/targets.go

@@ -190,9 +190,9 @@ func NewDescriptorResolver(descs []catalog.Descriptor) (*DescriptorResolver, err
 			if !ok {
 				return errors.Errorf("schema %d not found for desc %d", scID, desc.GetID())
 			}
-			scDesc, ok := scDescI.(catalog.SchemaDescriptor)
-			if !ok {
-				return errors.Errorf("descriptor %d is not a schema", scDescI.GetID())
+			scDesc, err := catalog.AsSchemaDescriptor(scDescI)
+			if err != nil {
+				return err
 			}
 			scName = scDesc.GetName()
 		}

pkg/ccl/backupccl/restore_job.go

@@ -433,23 +433,16 @@ func WriteDescriptors(
 			}
 			return err
 		}
-		// TODO(ajwerner): Utilize validation inside of the descs.Collection
-		// rather than reaching into the store.
-		dg := catalogkv.NewOneLevelUncachedDescGetter(txn, codec)
+
+		bdg := catalogkv.NewOneLevelUncachedDescGetter(txn, codec)
+		descs := make([]catalog.Descriptor, 0, len(databases)+len(tables))
 		for _, table := range tables {
-			if err := table.Validate(ctx, dg); err != nil {
-				return errors.Wrapf(err,
-					"validate table %d", errors.Safe(table.GetID()))
-			}
+			descs = append(descs, table)
 		}
-
 		for _, db := range databases {
-			if err := db.Validate(ctx, dg); err != nil {
-				return errors.Wrapf(err,
-					"validate database %d", errors.Safe(db.GetID()))
-			}
+			descs = append(descs, db)
 		}
-		return nil
+		return catalog.ValidateSelfAndCrossReferences(ctx, bdg, descs...)
 	}()
 	return errors.Wrapf(err, "restoring table desc and namespace entries")
 }

pkg/ccl/changefeedccl/avro_test.go

@@ -59,7 +59,7 @@ func parseTableDesc(createTableStmt string) (catalog.TableDescriptor, error) {
 	if err != nil {
 		return nil, err
 	}
-	return mutDesc, mutDesc.ValidateSelf(ctx)
+	return mutDesc, catalog.ValidateSelf(mutDesc)
 }
 
 func parseValues(tableDesc catalog.TableDescriptor, values string) ([]rowenc.EncDatumRow, error) {

pkg/ccl/importccl/import_stmt.go

@@ -1280,10 +1280,6 @@ func writeNonDropDatabaseChange(
 
 	queuedJob := []jobspb.JobID{job.ID()}
 	b := txn.NewBatch()
-	dg := catalogkv.NewOneLevelUncachedDescGetter(txn, p.ExecCfg().Codec)
-	if err := desc.Validate(ctx, dg); err != nil {
-		return nil, err
-	}
 	err = descsCol.WriteDescToBatch(
 		ctx,
 		p.ExtendedEvalContext().Tracing.KVTracingEnabled(),

pkg/ccl/logictestccl/testdata/logic_test/multi_region

@@ -911,7 +911,7 @@ SHOW ENUMS FROM drop_region_db
 schema  name                  values                         owner
 public  crdb_internal_region  {ap-southeast-2,ca-central-1}  root
 
-statement error pq: region "us-east-1" has not been added to database "drop_region_db"
+statement error pq: relation "t" \([0-9]+\): invalid locality config: region "us-east-1" has not been added to database "drop_region_db"
 CREATE TABLE drop_region_db.public.t(a int) LOCALITY REGIONAL BY TABLE IN "us-east-1"
 
 statement ok

pkg/ccl/partitionccl/BUILD.bazel

@@ -52,6 +52,7 @@ go_test(
         "//pkg/server",
         "//pkg/settings/cluster",
         "//pkg/sql",
+        "//pkg/sql/catalog",
         "//pkg/sql/catalog/catalogkv",
         "//pkg/sql/catalog/descpb",
         "//pkg/sql/catalog/tabledesc",

pkg/ccl/partitionccl/partition_test.go

@@ -29,6 +29,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
+	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/parser"
@@ -139,7 +140,7 @@ func (pt *partitioningTest) parse() error {
 			return err
 		}
 		pt.parsed.tableDesc = mutDesc
-		if err := pt.parsed.tableDesc.ValidateSelf(ctx); err != nil {
+		if err := catalog.ValidateSelf(pt.parsed.tableDesc); err != nil {
 			return err
 		}
 	}

pkg/cli/testdata/doctor/testcluster

@@ -2,6 +2,6 @@ doctor cluster
 ----
 debug doctor cluster
 Examining 35 descriptors and 36 namespace entries...
-   Table  53: ParentID  50, ParentSchemaID 29, Name 'foo': not being dropped but no namespace entry found
+  ParentID  50, ParentSchemaID 29: relation "foo" (53): not being dropped but no namespace entry found
 Examining 1 running jobs...
 ERROR: validation failed

pkg/cli/testdata/doctor/testzipdir

@@ -2,12 +2,12 @@ doctor zipdir
 ----
 debug doctor zipdir testdata/doctor/debugzip
 Examining 38 descriptors and 43 namespace entries...
-   Table  53: ParentID  52, ParentSchemaID 29, Name 'users': desc 53: parentID 52 does not exist
-   Table  54: ParentID  52, ParentSchemaID 29, Name 'vehicles': desc 54: parentID 52 does not exist
-   Table  55: ParentID  52, ParentSchemaID 29, Name 'rides': desc 55: parentID 52 does not exist
-   Table  56: ParentID  52, ParentSchemaID 29, Name 'vehicle_location_histories': desc 56: parentID 52 does not exist
-   Table  57: ParentID  52, ParentSchemaID 29, Name 'promo_codes': desc 57: parentID 52 does not exist
-   Table  58: ParentID  52, ParentSchemaID 29, Name 'user_promo_codes': desc 58: parentID 52 does not exist
+  ParentID  52, ParentSchemaID 29: relation "users" (53): referenced database ID 52: descriptor not found
+  ParentID  52, ParentSchemaID 29: relation "vehicles" (54): referenced database ID 52: descriptor not found
+  ParentID  52, ParentSchemaID 29: relation "rides" (55): referenced database ID 52: descriptor not found
+  ParentID  52, ParentSchemaID 29: relation "vehicle_location_histories" (56): referenced database ID 52: descriptor not found
+  ParentID  52, ParentSchemaID 29: relation "promo_codes" (57): referenced database ID 52: descriptor not found
+  ParentID  52, ParentSchemaID 29: relation "user_promo_codes" (58): referenced database ID 52: descriptor not found
 Descriptor 52: has namespace row(s) [{ParentID:0 ParentSchemaID:0 Name:movr}] but no descriptor
 Examining 1 running jobs...
 job 587337426984566785: schema change GC refers to missing table descriptor(s) [59]

pkg/server/testserver.go

@@ -274,9 +274,6 @@ func makeTestConfigFromParams(params base.TestServerArgs) Config {
 	if params.Knobs.SQLExecutor == nil {
 		cfg.TestingKnobs.SQLExecutor = &sql.ExecutorTestingKnobs{}
 	}
-	if !params.DisableTestingDescriptorValidation {
-		cfg.TestingKnobs.SQLExecutor.(*sql.ExecutorTestingKnobs).TestingDescriptorValidation = true
-	}
 
 	// For test servers, leave interleaved tables enabled by default. We'll remove
 	// this when we remove interleaved tables altogether.

pkg/sql/alter_column_type_test.go

@@ -404,7 +404,7 @@ ALTER TABLE t.test ALTER COLUMN x TYPE STRING;
 
 	<-childJobStartNotification
 
-	expected := "pq: unimplemented: cannot perform a schema change operation while an ALTER COLUMN TYPE schema change is in progress"
+	expected := `pq: relation "test" \(53\): unimplemented: cannot perform a schema change operation while an ALTER COLUMN TYPE schema change is in progress`
 	sqlDB.ExpectErr(t, expected, `
 ALTER TABLE t.test ADD COLUMN y INT;
 	`)

pkg/sql/alter_database.go

@@ -17,7 +17,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
@@ -219,8 +218,7 @@ func (n *alterDatabaseAddRegionNode) startExec(params runParams) error {
 
 	// Validate the type descriptor after the changes. We have to do this explicitly here, because
 	// we're using an internal call to addEnumValue above which doesn't perform validation.
-	dg := catalogkv.NewOneLevelUncachedDescGetter(params.p.txn, params.ExecCfg().Codec)
-	if err := typeDesc.Validate(params.ctx, dg); err != nil {
+	if err := validateDescriptor(params.ctx, params.p, typeDesc); err != nil {
 		return err
 	}
 
@@ -522,12 +520,6 @@ func (n *alterDatabasePrimaryRegionNode) switchPrimaryRegion(params runParams) e
 		return err
 	}
 
-	// Validate the type descriptor after the changes.
-	dg := catalogkv.NewOneLevelUncachedDescGetter(params.p.txn, params.ExecCfg().Codec)
-	if err := typeDesc.Validate(params.ctx, dg); err != nil {
-		return err
-	}
-
 	// Update the database's zone configuration.
 	if err := ApplyZoneConfigFromDatabaseRegionConfig(
 		params.ctx,

pkg/sql/alter_table.go

@@ -21,7 +21,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/resolver"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
@@ -281,7 +280,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 			case *tree.CheckConstraintTableDef:
 				var err error
 				params.p.runWithOptions(resolveFlags{contextDatabaseID: n.tableDesc.ParentID}, func() {
-					info, infoErr := n.tableDesc.GetConstraintInfo(params.ctx, nil)
+					info, infoErr := n.tableDesc.GetConstraintInfo()
 					if infoErr != nil {
 						err = infoErr
 						return
@@ -663,14 +662,13 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState,
 					"column %q in the middle of being added, try again later", t.Column)
 			}
-			if err := n.tableDesc.Validate(
-				params.ctx, catalogkv.NewOneLevelUncachedDescGetter(params.p.Txn(), params.ExecCfg().Codec),
-			); err != nil {
+
+			if err := validateDescriptor(params.ctx, params.p, n.tableDesc); err != nil {
 				return err
 			}
 
 		case *tree.AlterTableDropConstraint:
-			info, err := n.tableDesc.GetConstraintInfo(params.ctx, nil)
+			info, err := n.tableDesc.GetConstraintInfo()
 			if err != nil {
 				return err
 			}
@@ -692,14 +690,12 @@ func (n *alterTableNode) startExec(params runParams) error {
 				return err
 			}
 			descriptorChanged = true
-			if err := n.tableDesc.Validate(
-				params.ctx, catalogkv.NewOneLevelUncachedDescGetter(params.p.Txn(), params.ExecCfg().Codec),
-			); err != nil {
+			if err := validateDescriptor(params.ctx, params.p, n.tableDesc); err != nil {
 				return err
 			}
 
 		case *tree.AlterTableValidateConstraint:
-			info, err := n.tableDesc.GetConstraintInfo(params.ctx, nil)
+			info, err := n.tableDesc.GetConstraintInfo()
 			if err != nil {
 				return err
 			}
@@ -889,7 +885,7 @@ func (n *alterTableNode) startExec(params runParams) error {
 			descriptorChanged = descriptorChanged || descChanged
 
 		case *tree.AlterTableRenameConstraint:
-			info, err := n.tableDesc.GetConstraintInfo(params.ctx, nil)
+			info, err := n.tableDesc.GetConstraintInfo()
 			if err != nil {
 				return err
 			}
@@ -1106,7 +1102,7 @@ func applyColumnMutation(
 			}
 		}
 
-		info, err := tableDesc.GetConstraintInfo(params.ctx, nil)
+		info, err := tableDesc.GetConstraintInfo()
 		if err != nil {
 			return err
 		}
@@ -1141,7 +1137,7 @@ func applyColumnMutation(
 					"constraint in the middle of being dropped")
 			}
 		}
-		info, err := tableDesc.GetConstraintInfo(params.ctx, nil)
+		info, err := tableDesc.GetConstraintInfo()
 		if err != nil {
 			return err
 		}

pkg/sql/alter_table_locality.go

@@ -15,7 +15,6 @@ import (
 	"fmt"
 
 	"github.com/cockroachdb/cockroach/pkg/kv"
-	"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
@@ -135,7 +134,7 @@ func (n *alterTableSetLocalityNode) alterTableLocalityGlobalToRegionalByTable(
 
 	// Finalize the alter by writing a new table descriptor and updating the zone
 	// configuration.
-	if err := n.validateAndWriteNewTableLocalityAndZoneConfig(
+	if err := n.writeNewTableLocalityAndZoneConfig(
 		params,
 		n.dbDesc,
 	); err != nil {
@@ -167,7 +166,7 @@ func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToGlobal(
 
 	// Finalize the alter by writing a new table descriptor and updating the zone
 	// configuration.
-	if err := n.validateAndWriteNewTableLocalityAndZoneConfig(
+	if err := n.writeNewTableLocalityAndZoneConfig(
 		params,
 		n.dbDesc,
 	); err != nil {
@@ -208,7 +207,7 @@ func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToRegionalB
 	}
 
 	// Finalize the alter by writing a new table descriptor and updating the zone configuration.
-	if err := n.validateAndWriteNewTableLocalityAndZoneConfig(
+	if err := n.writeNewTableLocalityAndZoneConfig(
 		params,
 		n.dbDesc,
 	); err != nil {
@@ -539,21 +538,11 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error {
 		})
 }
 
-// validateAndWriteNewTableLocalityAndZoneConfig validates the newly updated
-// LocalityConfig in a table descriptor, writes that table descriptor, and
-// writes a new zone configuration for the given table.
-func (n *alterTableSetLocalityNode) validateAndWriteNewTableLocalityAndZoneConfig(
+// writeNewTableLocalityAndZoneConfig writes the table descriptor with the newly
+// updated LocalityConfig and writes a new zone configuration for the table.
+func (n *alterTableSetLocalityNode) writeNewTableLocalityAndZoneConfig(
 	params runParams, dbDesc *dbdesc.Immutable,
 ) error {
-	// Validate the new locality before updating the table descriptor.
-	dg := catalogkv.NewOneLevelUncachedDescGetter(params.p.txn, params.EvalContext().Codec)
-	if err := n.tableDesc.ValidateTableLocalityConfig(
-		params.ctx,
-		dg,
-	); err != nil {
-		return err
-	}
-
 	// Write out the table descriptor update.
 	if err := params.p.writeSchemaChange(
 		params.ctx,

pkg/sql/alter_type.go

@@ -126,12 +126,6 @@ func (n *alterTypeNode) startExec(params runParams) error {
 		return err
 	}
 
-	// Validate the type descriptor after the changes.
-	dg := catalogkv.NewOneLevelUncachedDescGetter(params.p.txn, params.ExecCfg().Codec)
-	if err := n.desc.Validate(params.ctx, dg); err != nil {
-		return err
-	}
-
 	if !eventLogDone {
 		// Write a log event.
 		if err := params.p.logEvent(params.ctx,

pkg/sql/catalog/BUILD.bazel

@@ -11,6 +11,7 @@ go_library(
         "errors.go",
         "table_col_map.go",
         "table_col_set.go",
+        "validate.go",
     ],
     importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog",
     visibility = ["//visibility:public"],