Skip to content

Commit

Permalink
sql,descs: add & adopt descriptor_validation session var
Browse files Browse the repository at this point in the history
This commit removes the `sql.catalog.descs.validate_on_write.enabled`
cluster setting and replaces it with the `descriptor_validation` session
variable. The default value is 'on' which indicates that catalog
descriptors are to be validated when both read from- and written to
the system.descriptor table. Other possible values are 'off' which
disables validation entirely and 'read_only' which disables it for
writes.

This commit therefore plumbs the session data into the descs.Collection
object and for this purpose defines a new DescriptorSessionDataProvider
interface, which is implemented in a new catsessiondata package.

Informs cockroachdb#50651.

Release note (sql change): added a new 'descriptor_validation' session
variable which can be set to 'read_only' or 'off' to disable descriptor
validation, which may be useful when mitigating or recovering from
catalog corruption.
  • Loading branch information
Marius Posta committed Oct 28, 2022
1 parent 5ff234c commit 0becdf9
Show file tree
Hide file tree
Showing 58 changed files with 631 additions and 273 deletions.
2 changes: 2 additions & 0 deletions pkg/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -1364,6 +1364,7 @@ GO_TARGETS = [
"//pkg/sql/catalog/catpb:catpb_test",
"//pkg/sql/catalog/catprivilege:catprivilege",
"//pkg/sql/catalog/catprivilege:catprivilege_test",
"//pkg/sql/catalog/catsessiondata:catsessiondata",
"//pkg/sql/catalog/colinfo/colinfotestutils:colinfotestutils",
"//pkg/sql/catalog/colinfo:colinfo",
"//pkg/sql/catalog/colinfo:colinfo_test",
Expand Down Expand Up @@ -2599,6 +2600,7 @@ GET_X_DATA_TARGETS = [
"//pkg/sql/catalog/catformat:get_x_data",
"//pkg/sql/catalog/catpb:get_x_data",
"//pkg/sql/catalog/catprivilege:get_x_data",
"//pkg/sql/catalog/catsessiondata:get_x_data",
"//pkg/sql/catalog/colinfo:get_x_data",
"//pkg/sql/catalog/colinfo/colinfotestutils:get_x_data",
"//pkg/sql/catalog/dbdesc:get_x_data",
Expand Down
5 changes: 2 additions & 3 deletions pkg/ccl/changefeedccl/avro_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,13 @@ import (

"github.com/cockroachdb/apd/v3"
"github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl/cdcevent"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/colinfo"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemaexpr"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
Expand Down Expand Up @@ -88,7 +87,7 @@ func parseTableDesc(createTableStmt string) (catalog.TableDescriptor, error) {
mutDesc.Families = []descpb.ColumnFamilyDescriptor{
{ID: primary, Name: "primary", ColumnIDs: mutDesc.PublicColumnIDs(), ColumnNames: columnNames},
}
return mutDesc, descbuilder.ValidateSelf(mutDesc, clusterversion.TestingClusterVersion)
return mutDesc, desctestutils.TestingValidateSelf(mutDesc)
}

func parseValues(tableDesc catalog.TableDescriptor, values string) ([]rowenc.EncDatumRow, error) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/changefeedccl/cdcevent/rowfetcher_cache.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func newRowFetcherCache(
return &rowFetcherCache{
codec: codec,
leaseMgr: leaseMgr,
collection: cf.NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* monitor */),
collection: cf.NewCollection(ctx),
db: db,
fetchers: cache.NewUnorderedCache(DefaultCacheConfig),
watchedFamilies: watchedFamilies,
Expand Down
2 changes: 1 addition & 1 deletion pkg/ccl/changefeedccl/changefeed_stmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -1167,7 +1167,7 @@ func getQualifiedTableName(
func getQualifiedTableNameObj(
ctx context.Context, execCfg *sql.ExecutorConfig, txn *kv.Txn, desc catalog.TableDescriptor,
) (tree.TableName, error) {
col := execCfg.CollectionFactory.NewCollection(ctx, nil /* TemporarySchemaProvider */, nil /* monitor */)
col := execCfg.CollectionFactory.NewCollection(ctx)
dbDesc, err := col.Direct().MustGetDatabaseDescByID(ctx, txn, desc.GetParentID())
if err != nil {
return tree.TableName{}, err
Expand Down
2 changes: 0 additions & 2 deletions pkg/ccl/partitionccl/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ go_test(
"//pkg/ccl/storageccl",
"//pkg/ccl/testutilsccl",
"//pkg/ccl/utilccl",
"//pkg/clusterversion",
"//pkg/config",
"//pkg/config/zonepb",
"//pkg/jobs",
Expand All @@ -62,7 +61,6 @@ go_test(
"//pkg/sql/catalog",
"//pkg/sql/catalog/bootstrap",
"//pkg/sql/catalog/catalogkeys",
"//pkg/sql/catalog/descbuilder",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/desctestutils",
"//pkg/sql/catalog/tabledesc",
Expand Down
4 changes: 1 addition & 3 deletions pkg/ccl/partitionccl/partition_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ import (
"github.com/cockroachdb/cockroach-go/v2/crdb"
"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/config"
"github.com/cockroachdb/cockroach/pkg/config/zonepb"
"github.com/cockroachdb/cockroach/pkg/jobs"
Expand All @@ -33,7 +32,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/bootstrap"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
Expand Down Expand Up @@ -155,7 +153,7 @@ func (pt *partitioningTest) parse() error {
return err
}
pt.parsed.tableDesc = mutDesc
if err := descbuilder.ValidateSelf(pt.parsed.tableDesc, clusterversion.TestingClusterVersion); err != nil {
if err := desctestutils.TestingValidateSelf(pt.parsed.tableDesc); err != nil {
return err
}
}
Expand Down
1 change: 0 additions & 1 deletion pkg/jobs/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ go_test(
"//pkg/sql",
"//pkg/sql/catalog/catalogkeys",
"//pkg/sql/catalog/colinfo",
"//pkg/sql/catalog/descbuilder",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/catalog/desctestutils",
Expand Down
4 changes: 1 addition & 3 deletions pkg/jobs/registry_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,15 +22,13 @@ import (
"time"

"github.com/cockroachdb/cockroach/pkg/base"
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
"github.com/cockroachdb/cockroach/pkg/keys"
"github.com/cockroachdb/cockroach/pkg/kv"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/spanconfig"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/desctestutils"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
Expand Down Expand Up @@ -82,7 +80,7 @@ func writeMutation(
) {
tableDesc.Mutations = append(tableDesc.Mutations, m)
tableDesc.Version++
if err := descbuilder.ValidateSelf(tableDesc, clusterversion.TestingClusterVersion); err != nil {
if err := desctestutils.TestingValidateSelf(tableDesc); err != nil {
t.Fatal(err)
}
if err := kvDB.Put(
Expand Down
1 change: 1 addition & 0 deletions pkg/server/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,7 @@ go_library(
"//pkg/sql/cacheutil",
"//pkg/sql/catalog/bootstrap",
"//pkg/sql/catalog/catalogkeys",
"//pkg/sql/catalog/catsessiondata",
"//pkg/sql/catalog/colinfo",
"//pkg/sql/catalog/descidgen",
"//pkg/sql/catalog/descpb",
Expand Down
2 changes: 2 additions & 0 deletions pkg/server/server_sql.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/sql"
"github.com/cockroachdb/cockroach/pkg/sql/cacheutil"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descidgen"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/hydrateddesc"
Expand Down Expand Up @@ -658,6 +659,7 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
hydratedDescCache,
spanConfig.splitter,
spanConfig.limiter,
catsessiondata.DefaultDescriptorSessionDataProvider,
)

clusterIDForSQL := cfg.rpcContext.LogicalClusterID
Expand Down
1 change: 1 addition & 0 deletions pkg/settings/registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ var retiredSettings = map[string]struct{}{
"sql.ttl.default_range_concurrency": {},

// removed as of 23.1.
"sql.catalog.descs.validate_on_write.enabled": {},
"sql.distsql.max_running_flows": {},
"sql.distsql.flow_scheduler_queueing.enabled": {},
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/sql/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,7 @@ go_library(
"//pkg/sql/catalog/catformat",
"//pkg/sql/catalog/catpb",
"//pkg/sql/catalog/catprivilege",
"//pkg/sql/catalog/catsessiondata",
"//pkg/sql/catalog/colinfo",
"//pkg/sql/catalog/dbdesc",
"//pkg/sql/catalog/descbuilder",
Expand Down Expand Up @@ -687,7 +688,6 @@ go_test(
"//pkg/sql/catalog/bootstrap",
"//pkg/sql/catalog/catalogkeys",
"//pkg/sql/catalog/catpb",
"//pkg/sql/catalog/descbuilder",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/catalog/desctestutils",
Expand Down
6 changes: 4 additions & 2 deletions pkg/sql/alter_primary_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ import (
"github.com/cockroachdb/cockroach/pkg/settings/cluster"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/paramparse"
"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
Expand Down Expand Up @@ -544,7 +545,8 @@ func (p *planner) AlterPrimaryKey(
}
tableDesc.AddPrimaryKeySwapMutation(swapArgs)

if err := descbuilder.ValidateSelf(tableDesc, version); err != nil {
dvmp := catsessiondata.NewDescriptorSessionDataProvider(p.SessionData())
if err := descs.ValidateSelf(tableDesc, version, dvmp); err != nil {
return err
}

Expand Down
17 changes: 17 additions & 0 deletions pkg/sql/catalog/catsessiondata/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
load("//build/bazelutil/unused_checker:unused.bzl", "get_x_data")
load("@io_bazel_rules_go//go:def.bzl", "go_library")

go_library(
name = "catsessiondata",
srcs = ["descriptor_session_data_provider.go"],
importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/catsessiondata",
visibility = ["//visibility:public"],
deps = [
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/descs",
"//pkg/sql/sessiondata",
"//pkg/sql/sessiondatapb",
],
)

get_x_data(name = "get_x_data")
130 changes: 130 additions & 0 deletions pkg/sql/catalog/catsessiondata/descriptor_session_data_provider.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
// Copyright 2022 The Cockroach Authors.
//
// Use of this software is governed by the Business Source License
// included in the file licenses/BSL.txt.
//
// As of the Change Date specified in that file, in accordance with
// the Business Source License, use of this software will be governed
// by the Apache License, Version 2.0, included in the file
// licenses/APL.txt.

package catsessiondata

import (
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
"github.com/cockroachdb/cockroach/pkg/sql/sessiondatapb"
)

type sessionData sessiondata.SessionData
type sessionDataStack sessiondata.Stack

func (sds *sessionDataStack) top() *sessionData {
if sds == nil {
return nil
}
return (*sessionData)((*sessiondata.Stack)(sds).Top())
}

// DefaultDescriptorSessionDataProvider is the default implementation of
// descs.DescriptorSessionDataProvider
var DefaultDescriptorSessionDataProvider descs.DescriptorSessionDataProvider = (*sessionData)(nil)
var _ descs.DescriptorSessionDataProvider = (*sessionDataStack)(nil)

// NewDescriptorSessionDataProvider constructs a
// descs.DescriptorSessionDataProvider instance given session data.
// The pointer may be nil.
func NewDescriptorSessionDataProvider(
sd *sessiondata.SessionData,
) descs.DescriptorSessionDataProvider {
return (*sessionData)(sd)
}

// NewDescriptorSessionDataStackProvider constructs a
// descs.DescriptorSessionDataProvider instance given a pointer to a session
// data stack. The pointer may be nil.
func NewDescriptorSessionDataStackProvider(
sds *sessiondata.Stack,
) descs.DescriptorSessionDataProvider {
return (*sessionDataStack)(sds)
}

// HasTemporarySchema implements descs.TemporarySchemaProvider.
func (sd *sessionData) HasTemporarySchema() bool {
return sd != nil
}

// HasTemporarySchema implements descs.TemporarySchemaProvider.
func (sds *sessionDataStack) HasTemporarySchema() bool {
return sds.top() != nil
}

// GetTemporarySchemaName implements descs.TemporarySchemaProvider.
func (sd *sessionData) GetTemporarySchemaName() string {
if sd == nil {
return ""
}
return (*sessiondata.SessionData)(sd).SearchPath.GetTemporarySchemaName()
}

// GetTemporarySchemaName implements descs.TemporarySchemaProvider.
func (sds *sessionDataStack) GetTemporarySchemaName() string {
return sds.top().GetTemporarySchemaName()
}

// GetTemporarySchemaIDForDB implements descs.TemporarySchemaProvider.
func (sd *sessionData) GetTemporarySchemaIDForDB(dbID descpb.ID) descpb.ID {
if sd == nil {
return descpb.InvalidID
}
ret, _ := (*sessiondata.SessionData)(sd).GetTemporarySchemaIDForDB(uint32(dbID))
return descpb.ID(ret)
}

// GetTemporarySchemaIDForDB implements descs.TemporarySchemaProvider.
func (sds *sessionDataStack) GetTemporarySchemaIDForDB(dbID descpb.ID) descpb.ID {
return sds.top().GetTemporarySchemaIDForDB(dbID)
}

// MaybeGetDatabaseForTemporarySchemaID implements descs.TemporarySchemaProvider.
func (sd *sessionData) MaybeGetDatabaseForTemporarySchemaID(id descpb.ID) descpb.ID {
if sd == nil {
return descpb.InvalidID
}
ret, _ := (*sessiondata.SessionData)(sd).MaybeGetDatabaseForTemporarySchemaID(uint32(id))
return descpb.ID(ret)
}

// MaybeGetDatabaseForTemporarySchemaID implements descs.TemporarySchemaProvider.
func (sds *sessionDataStack) MaybeGetDatabaseForTemporarySchemaID(id descpb.ID) descpb.ID {
return sds.top().MaybeGetDatabaseForTemporarySchemaID(id)
}

// ValidateDescriptorsOnRead implements descs.DescriptorValidationModeProvider.
func (sd *sessionData) ValidateDescriptorsOnRead() bool {
var mode sessiondatapb.DescriptorValidationMode
if sd != nil {
mode = sd.DescriptorValidationMode
}
return mode != sessiondatapb.DescriptorValidationOff
}

// ValidateDescriptorsOnRead implements descs.DescriptorValidationModeProvider.
func (sds *sessionDataStack) ValidateDescriptorsOnRead() bool {
return sds.top().ValidateDescriptorsOnRead()
}

// ValidateDescriptorsOnWrite implements descs.DescriptorValidationModeProvider.
func (sd *sessionData) ValidateDescriptorsOnWrite() bool {
var mode sessiondatapb.DescriptorValidationMode
if sd != nil {
mode = sd.DescriptorValidationMode
}
return mode == sessiondatapb.DescriptorValidationOn
}

// ValidateDescriptorsOnWrite implements descs.DescriptorValidationModeProvider.
func (sds *sessionDataStack) ValidateDescriptorsOnWrite() bool {
return sds.top().ValidateDescriptorsOnWrite()
}
2 changes: 0 additions & 2 deletions pkg/sql/catalog/descbuilder/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,11 @@ go_library(
importpath = "github.com/cockroachdb/cockroach/pkg/sql/catalog/descbuilder",
visibility = ["//visibility:public"],
deps = [
"//pkg/clusterversion",
"//pkg/roachpb",
"//pkg/sql/catalog",
"//pkg/sql/catalog/dbdesc",
"//pkg/sql/catalog/descpb",
"//pkg/sql/catalog/funcdesc",
"//pkg/sql/catalog/internal/validate",
"//pkg/sql/catalog/schemadesc",
"//pkg/sql/catalog/tabledesc",
"//pkg/sql/catalog/typedesc",
Expand Down
7 changes: 0 additions & 7 deletions pkg/sql/catalog/descbuilder/desc_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,11 @@
package descbuilder

import (
"github.com/cockroachdb/cockroach/pkg/clusterversion"
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/funcdesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/internal/validate"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/schemadesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc"
"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
Expand Down Expand Up @@ -172,8 +170,3 @@ func BuildMutable(
}
return ret, nil
}

// ValidateSelf validates that the descriptor is internally consistent.
func ValidateSelf(desc catalog.Descriptor, version clusterversion.ClusterVersion) error {
return validate.Self(version, desc)
}
Loading

0 comments on commit 0becdf9

Please sign in to comment.