From 3245ca96a2fe02fee4ee04129b91072c7a3c2f9f Mon Sep 17 00:00:00 2001 From: Marius Posta Date: Wed, 24 Mar 2021 12:25:28 -0400 Subject: [PATCH] catalog: add telemetry for descriptor validation errors This commit adds `sql.schema.validation_errors.*` telemetry keys to descriptor validation errors. Fixes #61786. Release note: None --- pkg/server/telemetry/BUILD.bazel | 1 + pkg/server/telemetry/features.go | 10 +- pkg/sql/catalog/BUILD.bazel | 1 + pkg/sql/catalog/catalogkv/catalogkv.go | 2 +- pkg/sql/catalog/catconstants/constants.go | 4 + pkg/sql/catalog/dbdesc/database_test.go | 3 +- pkg/sql/catalog/descs/collection.go | 8 +- .../catalog/schemadesc/schema_desc_test.go | 3 +- pkg/sql/catalog/tabledesc/validate_test.go | 3 +- pkg/sql/catalog/validate.go | 240 +++++++++++++----- pkg/sql/doctor/doctor.go | 3 +- pkg/sql/testdata/telemetry/error | 13 + 12 files changed, 222 insertions(+), 69 deletions(-) diff --git a/pkg/server/telemetry/BUILD.bazel b/pkg/server/telemetry/BUILD.bazel index ba02d72a9546..10604c5c28a9 100644 --- a/pkg/server/telemetry/BUILD.bazel +++ b/pkg/server/telemetry/BUILD.bazel @@ -9,6 +9,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/server/telemetry", visibility = ["//visibility:public"], deps = [ + "//pkg/sql/catalog/catconstants", "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", "//pkg/util/metric", diff --git a/pkg/server/telemetry/features.go b/pkg/server/telemetry/features.go index 4ba9c329545c..3506534a5c48 100644 --- a/pkg/server/telemetry/features.go +++ b/pkg/server/telemetry/features.go @@ -13,8 +13,10 @@ package telemetry import ( "fmt" "math" + "strings" "sync/atomic" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/util/metric" @@ -247,9 +249,13 @@ func RecordError(err error) { default: prefix = "othererror." + code.String() + "." } - for _, tk := range tkeys { - Count(prefix + tk) + prefixedTelemetryKey := prefix + tk + if strings.HasPrefix(tk, catconstants.ValidationTelemetryKeyPrefix) { + // Descriptor validation errors already have their own prefixing scheme. + prefixedTelemetryKey = tk + } + Count(prefixedTelemetryKey) } } } diff --git a/pkg/sql/catalog/BUILD.bazel b/pkg/sql/catalog/BUILD.bazel index 15aa1a145860..5fb20f683010 100644 --- a/pkg/sql/catalog/BUILD.bazel +++ b/pkg/sql/catalog/BUILD.bazel @@ -22,6 +22,7 @@ go_library( "//pkg/kv", "//pkg/roachpb", "//pkg/settings/cluster", + "//pkg/sql/catalog/catconstants", "//pkg/sql/catalog/descpb", "//pkg/sql/pgwire/pgcode", "//pkg/sql/pgwire/pgerror", diff --git a/pkg/sql/catalog/catalogkv/catalogkv.go b/pkg/sql/catalog/catalogkv/catalogkv.go index 9d03c2a1fd71..80284db5b210 100644 --- a/pkg/sql/catalog/catalogkv/catalogkv.go +++ b/pkg/sql/catalog/catalogkv/catalogkv.go @@ -165,7 +165,7 @@ func descriptorFromKeyValue( } else { desc = b.BuildImmutable() } - err = catalog.Validate(ctx, dg, validationLevel, desc).CombinedError() + err = catalog.Validate(ctx, dg, catalog.ValidationReadTelemetry, validationLevel, desc).CombinedError() if err != nil { return nil, err } diff --git a/pkg/sql/catalog/catconstants/constants.go b/pkg/sql/catalog/catconstants/constants.go index 0236908cd6e8..11a2550a811b 100644 --- a/pkg/sql/catalog/catconstants/constants.go +++ b/pkg/sql/catalog/catconstants/constants.go @@ -198,3 +198,7 @@ const ( PgExtensionSpatialRefSysTableID MinVirtualID = PgExtensionSpatialRefSysTableID ) + +// ValidationTelemetryKeyPrefix is the prefix of telemetry keys pertaining to +// descriptor validation failures. +const ValidationTelemetryKeyPrefix = "sql.schema.validation_errors." diff --git a/pkg/sql/catalog/dbdesc/database_test.go b/pkg/sql/catalog/dbdesc/database_test.go index 3b0feb7f8ff6..7bfffefc0102 100644 --- a/pkg/sql/catalog/dbdesc/database_test.go +++ b/pkg/sql/catalog/dbdesc/database_test.go @@ -287,7 +287,8 @@ func TestValidateCrossDatabaseReferences(t *testing.T) { descs.Namespace[namespaceKey] = desc.GetID() } expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), test.err) - if err := catalog.Validate(ctx, descs, catalog.ValidationLevelAllPreTxnCommit, desc).CombinedError(); err == nil { + results := catalog.Validate(ctx, descs, catalog.NoValidationTelemetry, catalog.ValidationLevelAllPreTxnCommit, desc) + if err := results.CombinedError(); err == nil { if test.err != "" { t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, test.desc) } diff --git a/pkg/sql/catalog/descs/collection.go b/pkg/sql/catalog/descs/collection.go index 69f9a225bcb1..b0a3d874023f 100644 --- a/pkg/sql/catalog/descs/collection.go +++ b/pkg/sql/catalog/descs/collection.go @@ -1528,7 +1528,13 @@ func (tc *Collection) ValidateUncommittedDescriptors(ctx context.Context, txn *k return nil } bdg := catalogkv.NewOneLevelUncachedDescGetter(txn, tc.codec()) - return catalog.Validate(ctx, bdg, catalog.ValidationLevelAllPreTxnCommit, descs...).CombinedError() + return catalog.Validate( + ctx, + bdg, + catalog.ValidationWriteTelemetry, + catalog.ValidationLevelAllPreTxnCommit, + descs..., + ).CombinedError() } // User defined type accessors. diff --git a/pkg/sql/catalog/schemadesc/schema_desc_test.go b/pkg/sql/catalog/schemadesc/schema_desc_test.go index 60b81a421b79..e52b4ecfbc23 100644 --- a/pkg/sql/catalog/schemadesc/schema_desc_test.go +++ b/pkg/sql/catalog/schemadesc/schema_desc_test.go @@ -172,7 +172,8 @@ func TestValidateCrossSchemaReferences(t *testing.T) { descs.Descriptors[test.dbDesc.ID] = dbdesc.NewBuilder(&test.dbDesc).BuildImmutable() expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), test.err) const validateCrossReferencesOnly = catalog.ValidationLevelCrossReferences &^ (catalog.ValidationLevelCrossReferences >> 1) - if err := catalog.Validate(ctx, descs, validateCrossReferencesOnly, desc).CombinedError(); err == nil { + results := catalog.Validate(ctx, descs, catalog.NoValidationTelemetry, validateCrossReferencesOnly, desc) + if err := results.CombinedError(); err == nil { if test.err != "" { t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, test.desc) } diff --git a/pkg/sql/catalog/tabledesc/validate_test.go b/pkg/sql/catalog/tabledesc/validate_test.go index 874799cc628b..25451ada29e2 100644 --- a/pkg/sql/catalog/tabledesc/validate_test.go +++ b/pkg/sql/catalog/tabledesc/validate_test.go @@ -1485,7 +1485,8 @@ func TestValidateCrossTableReferences(t *testing.T) { desc := NewBuilder(&test.desc).BuildImmutable() expectedErr := fmt.Sprintf("%s %q (%d): %s", desc.DescriptorType(), desc.GetName(), desc.GetID(), test.err) const validateCrossReferencesOnly = catalog.ValidationLevelCrossReferences &^ (catalog.ValidationLevelCrossReferences >> 1) - if err := catalog.Validate(ctx, descs, validateCrossReferencesOnly, desc).CombinedError(); err == nil { + results := catalog.Validate(ctx, descs, catalog.NoValidationTelemetry, validateCrossReferencesOnly, desc) + if err := results.CombinedError(); err == nil { if test.err != "" { t.Errorf("%d: expected \"%s\", but found success: %+v", i, expectedErr, test.desc) } diff --git a/pkg/sql/catalog/validate.go b/pkg/sql/catalog/validate.go index c62bcf9195ce..28b590439012 100644 --- a/pkg/sql/catalog/validate.go +++ b/pkg/sql/catalog/validate.go @@ -12,8 +12,9 @@ package catalog import ( "context" - "fmt" + "strings" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/catconstants" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/errors" ) @@ -30,31 +31,21 @@ import ( func Validate( ctx context.Context, maybeBatchDescGetter DescGetter, - level ValidationLevel, + telemetry ValidationTelemetry, + targetLevel ValidationLevel, descriptors ...Descriptor, ) ValidationErrors { - // Check internal descriptor consistency. - var vea validationErrorAccumulator - handleDescGetterError := func(descGetterErr error, errPrefix string) { - // Contrary to errors collected during Validate via vea.Report(), - // the descGetterErr may be a transaction error, which may trigger retries. - // It's therefore important that the combined error produced by the - // returned ValidationErrors interface unwraps to descGetterErr. For this - // reason we place it at the head of the errors slice. - vea.errors = append(make([]error, 1, 1+len(vea.errors)), vea.errors...) - vea.errors[0] = errors.Wrapf(descGetterErr, "%s", errPrefix) - } - if level == NoValidation { - return &vea - } - for _, desc := range descriptors { - if level&ValidationLevelSelfOnly == 0 { - continue - } - vea.setPrefix(desc) - desc.ValidateSelf(&vea) - } - if level <= ValidationLevelSelfOnly || len(vea.errors) > 0 { + vea := validationErrorAccumulator{ + ValidationTelemetry: telemetry, + targetLevel: targetLevel, + } + // Internal descriptor consistency checks. + if !vea.validateDescriptorsAtLevel( + ValidationLevelSelfOnly, + descriptors, + func(desc Descriptor) { + desc.ValidateSelf(&vea) + }) { return &vea } // Collect descriptors referenced by the validated descriptors. @@ -62,44 +53,46 @@ func Validate( // special cases those neighbors' immediate neighbors also. vdg, descGetterErr := collectDescriptorsForValidation(ctx, maybeBatchDescGetter, descriptors) if descGetterErr != nil { - handleDescGetterError(descGetterErr, "collecting referenced descriptors") + vea.reportDescGetterError(collectingReferencedDescriptors, descGetterErr) return &vea } - // Perform cross-reference checks. - for _, desc := range descriptors { - if level&ValidationLevelCrossReferences == 0 || desc.Dropped() { - continue - } - vea.setPrefix(desc) - desc.ValidateCrossReferences(&vea, vdg) - } - if level <= ValidationLevelCrossReferences { + // Descriptor cross-reference checks. + if !vea.validateDescriptorsAtLevel( + ValidationLevelCrossReferences, + descriptors, + func(desc Descriptor) { + if !desc.Dropped() { + desc.ValidateCrossReferences(&vea, vdg) + } + }) { return &vea } - if level&ValidationLevelNamespace != 0 { - // Collect descriptor namespace table entries. + // Collect descriptor namespace table entries, if running namespace checks. + if ValidationLevelNamespace&targetLevel != 0 { descGetterErr = vdg.addNamespaceEntries(ctx, descriptors, maybeBatchDescGetter) if descGetterErr != nil { - handleDescGetterError(descGetterErr, "collecting namespace table entries") + vea.reportDescGetterError(collectingNamespaceEntries, descGetterErr) return &vea } - // Perform Namespace checks - for _, desc := range descriptors { - vea.setPrefix(desc) - validateNamespace(desc, &vea, vdg.Namespace) - } } - if level <= ValidationLevelNamespace { + // Namespace validation checks + if !vea.validateDescriptorsAtLevel( + ValidationLevelNamespace, + descriptors, + func(desc Descriptor) { + validateNamespace(desc, &vea, vdg.Namespace) + }) { return &vea } - // Perform pre-txn-commit checks. - for _, desc := range descriptors { - if level&ValidationLevelAllPreTxnCommit == 0 || desc.Dropped() { - continue - } - vea.setPrefix(desc) - desc.ValidateTxnCommit(&vea, vdg) - } + // Descriptor pre-txn-commit checks. + _ = vea.validateDescriptorsAtLevel( + ValidationLevelAllPreTxnCommit, + descriptors, + func(desc Descriptor) { + if !desc.Dropped() { + desc.ValidateTxnCommit(&vea, vdg) + } + }) return &vea } @@ -118,14 +111,32 @@ const ( // table records. ValidationLevelNamespace // ValidationLevelAllPreTxnCommit means do the above and also perform - // pre-txn-commit checks. + // pre-txn-commit checks. This is the level of validation required when + // writing a descriptor to storage. + // Errors accumulated when validating up to this level come with additional + // telemetry. ValidationLevelAllPreTxnCommit ) +// ValidationTelemetry defines the kind of telemetry keys to add to the errors. +type ValidationTelemetry int + +const ( + // NoValidationTelemetry means no telemetry keys are added. + NoValidationTelemetry ValidationTelemetry = iota + // ValidationReadTelemetry means telemetry keys are added for descriptor + // reads. + ValidationReadTelemetry + // ValidationWriteTelemetry means telemetry keys are added for descriptor + // writes. + ValidationWriteTelemetry +) + // ValidateSelf is a convenience function for validate called at the // ValidationLevelSelfOnly level and combining the resulting errors. func ValidateSelf(descriptors ...Descriptor) error { - return Validate(context.TODO(), nil, ValidationLevelSelfOnly, descriptors...).CombinedError() + results := Validate(context.TODO(), nil, NoValidationTelemetry, ValidationLevelSelfOnly, descriptors...) + return results.CombinedError() } // ValidateSelfAndCrossReferences is a convenience function for Validate called at the @@ -133,7 +144,8 @@ func ValidateSelf(descriptors ...Descriptor) error { func ValidateSelfAndCrossReferences( ctx context.Context, maybeBatchDescGetter DescGetter, descriptors ...Descriptor, ) error { - return Validate(ctx, maybeBatchDescGetter, ValidationLevelCrossReferences, descriptors...).CombinedError() + results := Validate(ctx, maybeBatchDescGetter, NoValidationTelemetry, ValidationLevelCrossReferences, descriptors...) + return results.CombinedError() } // ValidationErrorAccumulator is used by the validation methods on Descriptor @@ -182,17 +194,46 @@ func (ve *validationErrors) Errors() []error { // CombinedError implements the ValidationErrors interface. func (ve *validationErrors) CombinedError() error { var combinedErr error + var extraTelemetryKeys []string for i := len(ve.errors) - 1; i >= 0; i-- { combinedErr = errors.CombineErrors(ve.errors[i], combinedErr) } - return combinedErr + // Decorate the combined error with all validation telemetry keys. + // Otherwise, those not in the causal chain will be ignored. + for _, err := range ve.errors { + for _, key := range errors.GetTelemetryKeys(err) { + if strings.HasPrefix(key, catconstants.ValidationTelemetryKeyPrefix) { + extraTelemetryKeys = append(extraTelemetryKeys, key) + } + } + } + if extraTelemetryKeys == nil { + return combinedErr + } + return errors.WithTelemetry(combinedErr, extraTelemetryKeys...) } type validationErrorAccumulator struct { + // Accumulated errors end up in here. validationErrors - wrapPrefix string + + // The remaining fields represent the internal state of the Validate function + // Used to decorate errors with appropriate prefixes and telemetry keys. + ValidationTelemetry // set at initialization + targetLevel ValidationLevel // set at initialization + currentState validationErrorAccumulatorState + currentLevel ValidationLevel + currentDescriptor Descriptor } +type validationErrorAccumulatorState int + +const ( + validatingDescriptor validationErrorAccumulatorState = iota + collectingReferencedDescriptors + collectingNamespaceEntries +) + var _ ValidationErrorAccumulator = &validationErrorAccumulator{} // Report implements the ValidationErrorAccumulator interface. @@ -200,14 +241,91 @@ func (vea *validationErrorAccumulator) Report(err error) { if err == nil { return } - if vea.wrapPrefix != "" { - err = errors.Wrapf(err, "%s", vea.wrapPrefix) + vea.errors = append(vea.errors, vea.decorate(err)) +} + +func (vea *validationErrorAccumulator) validateDescriptorsAtLevel( + level ValidationLevel, descs []Descriptor, validationFn func(descriptor Descriptor), +) bool { + vea.currentState = validatingDescriptor + vea.currentLevel = level + if vea.currentLevel&vea.targetLevel != 0 { + for _, desc := range descs { + vea.currentDescriptor = desc + validationFn(desc) + } } - vea.errors = append(vea.errors, err) + vea.currentDescriptor = nil // ensures we don't needlessly hold a reference. + if len(vea.errors) > 0 { + return false + } + if vea.targetLevel <= vea.currentLevel { + return false + } + return true +} + +func (vea *validationErrorAccumulator) reportDescGetterError( + state validationErrorAccumulatorState, err error, +) { + vea.currentState = state + // Contrary to errors collected during Validate via vea.Report(), this error + // may be a transaction error, which may trigger retries. It's therefore + // important that the combined error produced by the returned ValidationErrors + // interface unwraps to this error. For this reason we place it at the head of + // the errors slice. + vea.errors = append(make([]error, 1, 1+len(vea.errors)), vea.errors...) + vea.errors[0] = vea.decorate(err) } -func (vea *validationErrorAccumulator) setPrefix(desc Descriptor) { - vea.wrapPrefix = fmt.Sprintf("%s %q (%d)", desc.DescriptorType(), desc.GetName(), desc.GetID()) +func (vea *validationErrorAccumulator) decorate(err error) error { + var tkSuffix string + switch vea.currentState { + case collectingReferencedDescriptors: + err = errors.Wrap(err, "collecting referenced descriptors") + tkSuffix = "read_referenced_descriptors" + case collectingNamespaceEntries: + err = errors.Wrap(err, "collecting namespace table entries") + tkSuffix = "read_namespace_table" + case validatingDescriptor: + name := vea.currentDescriptor.GetName() + id := vea.currentDescriptor.GetID() + // This contrived switch case is required to make the linter happy. + switch vea.currentDescriptor.DescriptorType() { + case Table: + err = errors.Wrapf(err, Table+" %q (%d)", name, id) + case Database: + err = errors.Wrapf(err, Database+" %q (%d)", name, id) + case Schema: + err = errors.Wrapf(err, Schema+" %q (%d)", name, id) + case Type: + err = errors.Wrapf(err, Type+" %q (%d)", name, id) + default: + return err + } + switch vea.currentLevel { + case ValidationLevelSelfOnly: + tkSuffix = "self" + case ValidationLevelCrossReferences: + tkSuffix = "cross_references" + case ValidationLevelNamespace: + tkSuffix = "namespace" + case ValidationLevelAllPreTxnCommit: + tkSuffix = "pre_txn_commit" + default: + return err + } + tkSuffix += "." + string(vea.currentDescriptor.DescriptorType()) + } + switch vea.ValidationTelemetry { + case ValidationReadTelemetry: + tkSuffix = "read." + tkSuffix + case ValidationWriteTelemetry: + tkSuffix = "write." + tkSuffix + default: + return err + } + return errors.WithTelemetry(err, catconstants.ValidationTelemetryKeyPrefix+tkSuffix) } // ValidationDescGetter is used by the validation methods on Descriptor. diff --git a/pkg/sql/doctor/doctor.go b/pkg/sql/doctor/doctor.go index 754707badbf7..ccb9a77d9d8d 100644 --- a/pkg/sql/doctor/doctor.go +++ b/pkg/sql/doctor/doctor.go @@ -161,7 +161,8 @@ func validateSafely( errs = append(errs, err) } }() - errs = append(errs, catalog.Validate(ctx, descGetter, catalog.ValidationLevelNamespace, desc).Errors()...) + results := catalog.Validate(ctx, descGetter, catalog.NoValidationTelemetry, catalog.ValidationLevelAllPreTxnCommit, desc) + errs = append(errs, results.Errors()...) return errs } diff --git a/pkg/sql/testdata/telemetry/error b/pkg/sql/testdata/telemetry/error index 96ee9889ccb5..549f44bd2073 100644 --- a/pkg/sql/testdata/telemetry/error +++ b/pkg/sql/testdata/telemetry/error @@ -4,6 +4,7 @@ feature-allowlist othererror.* errorcodes.* unimplemented.* +sql.schema.validation_errors.* ---- # 42601 is pgcode.Syntax. @@ -65,3 +66,15 @@ SELECT 2/0 ---- error: pq: division by zero errorcodes.22012 + +# Descriptor validation failure on transaction commit. +feature-usage +CREATE TABLE t (x INT PRIMARY KEY); +BEGIN; +ALTER TABLE t DROP CONSTRAINT "primary"; +COMMIT; +---- +error: pq: relation "t" (53): unimplemented: primary key dropped without subsequent addition of new primary key in same transaction +errorcodes.0A000 +sql.schema.validation_errors.write.pre_txn_commit.relation +unimplemented.#48026