Skip to content

Commit

Permalink
Fixes: #95764
Browse files Browse the repository at this point in the history
Previously, when renewing a lease we would fetch dependent
descriptors for cross-descriptor validation. This could
lead to perpetual transaction retries, in the schema where
there are tons of dependencies between descriptors since
lease renewals would happen in a high-priority transaction.
Additionally, this incurs additional latency for lease renewal.
To address this, this patch disables cross-validation by default for
lease renewals.

Epic: none
Release note (bug fix): Add support for disabling cross-descriptor validation
on lease renewal, which can be problematic when there are lots of descriptors
with lots of foreign key references, in which cases, the cross-reference
validation could starve schema changes. This can be enabled with
sql.catalog.descriptor_lease_renewal_cross_validation.
  • Loading branch information
fqazi committed Mar 1, 2023
1 parent 7fcbded commit 4b6691c
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 7 deletions.
4 changes: 4 additions & 0 deletions pkg/base/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ const (
// DefaultDescriptorLeaseRenewalTimeout is the default time
// before a lease expires when acquisition to renew the lease begins.
DefaultDescriptorLeaseRenewalTimeout = time.Minute

// DefaultLeaseRenewalCrossValidate is the default setting for if
// we should validate descriptors on lease renewals.
DefaultLeaseRenewalCrossValidate = true
)

// DefaultHistogramWindowInterval returns the default rotation window for
Expand Down
30 changes: 24 additions & 6 deletions pkg/sql/catalog/internal/catkv/descriptors.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func lookupDescriptorsAndValidate(
txn *kv.Txn,
cq catalogQuerier,
vd validate.ValidationDereferencer,
targetValidationLevel catalog.ValidationLevel,
ids []descpb.ID,
) ([]catalog.Descriptor, error) {
descs, err := lookupDescriptorsUnvalidated(ctx, txn, cq, ids)
Expand All @@ -68,7 +69,7 @@ func lookupDescriptorsAndValidate(
txn: txn,
}
}
ve := validate.Validate(ctx, version, vd, catalog.ValidationReadTelemetry, catalog.ValidationLevelCrossReferences, descs...)
ve := validate.Validate(ctx, version, vd, catalog.ValidationReadTelemetry, targetValidationLevel, descs...)
if err := ve.CombinedError(); err != nil {
return nil, err
}
Expand Down Expand Up @@ -138,21 +139,23 @@ func MaybeGetDescriptorByID(
expectedType: expectedType,
codec: codec,
}
descs, err := lookupDescriptorsAndValidate(ctx, version, txn, cq, vd, []descpb.ID{id})
descs, err := lookupDescriptorsAndValidate(ctx, version, txn, cq, vd, catalog.ValidationLevelCrossReferences, []descpb.ID{id})
if err != nil {
return nil, err
}
return descs[0], nil
}

// MustGetDescriptorsByID looks up the descriptors given their IDs,
// returning an error if any descriptor is not found.
func MustGetDescriptorsByID(
// MustGetDescriptorsByIDWithValidationLevel looks up the descriptors given their IDs,
// returning an error if any descriptor is not found. Descriptors are only
// validated till the specified validation level.
func MustGetDescriptorsByIDWithValidationLevel(
ctx context.Context,
version clusterversion.ClusterVersion,
codec keys.SQLCodec,
txn *kv.Txn,
vd validate.ValidationDereferencer,
targetValidationLevel catalog.ValidationLevel,
ids []descpb.ID,
expectedType catalog.DescriptorType,
) ([]catalog.Descriptor, error) {
Expand All @@ -161,7 +164,22 @@ func MustGetDescriptorsByID(
isRequired: true,
expectedType: expectedType,
}
return lookupDescriptorsAndValidate(ctx, version, txn, cq, vd, ids)
return lookupDescriptorsAndValidate(ctx, version, txn, cq, vd, targetValidationLevel, ids)
}

// MustGetDescriptorsByID looks up the descriptors given their IDs,
// returning an error if any descriptor is not found.
func MustGetDescriptorsByID(
ctx context.Context,
version clusterversion.ClusterVersion,
codec keys.SQLCodec,
txn *kv.Txn,
vd validate.ValidationDereferencer,
ids []descpb.ID,
expectedType catalog.DescriptorType,
) ([]catalog.Descriptor, error) {
return MustGetDescriptorsByIDWithValidationLevel(ctx, version, codec, txn, vd, catalog.ValidationLevelCrossReferences, ids, expectedType)

}

// MustGetDescriptorByID looks up the descriptor given its ID,
Expand Down
19 changes: 18 additions & 1 deletion pkg/sql/catalog/lease/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,14 @@ var LeaseRenewalDuration = settings.RegisterDurationSetting(
"controls the default time before a lease expires when acquisition to renew the lease begins",
base.DefaultDescriptorLeaseRenewalTimeout)

// LeaseRenewalCrossValidate controls if cross validation should be done during
// lease renewal.
var LeaseRenewalCrossValidate = settings.RegisterBoolSetting(
settings.TenantWritable,
"sql.catalog.descriptor_lease_renewal_cross_validation.enabled",
"controls if cross validation should be done during lease renewal",
base.DefaultLeaseRenewalCrossValidate)

func (s storage) leaseRenewalTimeout() time.Duration {
return LeaseRenewalDuration.Get(&s.settings.SV)
}
Expand All @@ -80,6 +88,10 @@ func (s storage) jitteredLeaseDuration() time.Duration {
2*jitterFraction*rand.Float64()))
}

func (s storage) crossValidateDuringRenewal() bool {
return LeaseRenewalCrossValidate.Get(&s.settings.SV)
}

// acquire a lease on the most recent version of a descriptor. If the lease
// cannot be obtained because the descriptor is in the process of being dropped
// or offline (currently only applicable to tables), the error will be of type
Expand Down Expand Up @@ -130,10 +142,15 @@ func (s storage) acquire(
}

version := s.settings.Version.ActiveVersion(ctx)
desc, err = catkv.MustGetDescriptorByID(ctx, version, s.codec, txn, nil /* vd */, id, catalog.Any)
var validationLevel catalog.ValidationLevel = catalog.ValidationLevelSelfOnly
if s.crossValidateDuringRenewal() {
validationLevel = catalog.ValidationLevelCrossReferences
}
descs, err := catkv.MustGetDescriptorsByIDWithValidationLevel(ctx, version, s.codec, txn, nil /* vd */, validationLevel, []descpb.ID{id}, catalog.Any)
if err != nil {
return err
}
desc = descs[0]
if err := catalog.FilterDescriptorState(
desc, tree.CommonLookupFlags{IncludeOffline: true}, // filter dropped only
); err != nil {
Expand Down

0 comments on commit 4b6691c

Please sign in to comment.