Skip to content

Commit

Permalink
sql: tenants don't unsplit manually split ranges on DROP {TABLE,INDEX}
Browse files Browse the repository at this point in the history
This requires us to scan the meta ranges, which is not something that
secondary tenants are allowed to do. Leaving the ranges split until
their sticky bit expires is fine, the existing code is just an
optimization.

This does serve as a reminder that:
1. we should make sure to remove sticky bits when we destroy a tenant's
   keyspace (see cockroachdb#48775).
2. we should probably not allow tenants to run `ALTER TABLE ... SPLIT AT`
   statements because we're not placing hard range count limits on tenants
   anywhere else. If others agree, I'll file an issue.
  • Loading branch information
nvanbenschoten committed Jul 29, 2020
1 parent 8f9e631 commit f4cdaab
Show file tree
Hide file tree
Showing 2 changed files with 39 additions and 30 deletions.
38 changes: 21 additions & 17 deletions pkg/sql/drop_index.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,27 +452,31 @@ func (p *planner) dropIndexByName(
for i, idxEntry := range tableDesc.Indexes {
if idxEntry.ID == idx.ID {
// Unsplit all manually split ranges in the index so they can be
// automatically merged by the merge queue.
span := tableDesc.IndexSpan(p.ExecCfg().Codec, idxEntry.ID)
ranges, err := ScanMetaKVs(ctx, p.txn, span)
if err != nil {
return err
}
for _, r := range ranges {
var desc roachpb.RangeDescriptor
if err := r.ValueProto(&desc); err != nil {
// automatically merged by the merge queue. Gate this on being the
// system tenant because secondary tenants aren't allowed to scan
// the meta ranges directly.
if p.ExecCfg().Codec.ForSystemTenant() {
span := tableDesc.IndexSpan(p.ExecCfg().Codec, idxEntry.ID)
ranges, err := ScanMetaKVs(ctx, p.txn, span)
if err != nil {
return err
}
// We have to explicitly check that the range descriptor's start key
// lies within the span of the index since ScanMetaKVs returns all
// intersecting spans.
if (desc.GetStickyBit() != hlc.Timestamp{}) && span.Key.Compare(desc.StartKey.AsRawKey()) <= 0 {
// Swallow "key is not the start of a range" errors because it would
// mean that the sticky bit was removed and merged concurrently. DROP
// INDEX should not fail because of this.
if err := p.ExecCfg().DB.AdminUnsplit(ctx, desc.StartKey); err != nil && !strings.Contains(err.Error(), "is not the start of a range") {
for _, r := range ranges {
var desc roachpb.RangeDescriptor
if err := r.ValueProto(&desc); err != nil {
return err
}
// We have to explicitly check that the range descriptor's start key
// lies within the span of the index since ScanMetaKVs returns all
// intersecting spans.
if (desc.GetStickyBit() != hlc.Timestamp{}) && span.Key.Compare(desc.StartKey.AsRawKey()) <= 0 {
// Swallow "key is not the start of a range" errors because it would
// mean that the sticky bit was removed and merged concurrently. DROP
// INDEX should not fail because of this.
if err := p.ExecCfg().DB.AdminUnsplit(ctx, desc.StartKey); err != nil && !strings.Contains(err.Error(), "is not the start of a range") {
return err
}
}
}
}

Expand Down
31 changes: 18 additions & 13 deletions pkg/sql/drop_table.go
Original file line number Diff line number Diff line change
Expand Up @@ -355,23 +355,28 @@ func (p *planner) initiateDropTable(
}

// Unsplit all manually split ranges in the table so they can be
// automatically merged by the merge queue.
ranges, err := ScanMetaKVs(ctx, p.txn, tableDesc.TableSpan(p.ExecCfg().Codec))
if err != nil {
return err
}
for _, r := range ranges {
var desc roachpb.RangeDescriptor
if err := r.ValueProto(&desc); err != nil {
// automatically merged by the merge queue. Gate this on being the
// system tenant because secondary tenants aren't allowed to scan
// the meta ranges directly.
if p.ExecCfg().Codec.ForSystemTenant() {
span := tableDesc.TableSpan(p.ExecCfg().Codec)
ranges, err := ScanMetaKVs(ctx, p.txn, span)
if err != nil {
return err
}
if (desc.GetStickyBit() != hlc.Timestamp{}) {
// Swallow "key is not the start of a range" errors because it would mean
// that the sticky bit was removed and merged concurrently. DROP TABLE
// should not fail because of this.
if err := p.ExecCfg().DB.AdminUnsplit(ctx, desc.StartKey); err != nil && !strings.Contains(err.Error(), "is not the start of a range") {
for _, r := range ranges {
var desc roachpb.RangeDescriptor
if err := r.ValueProto(&desc); err != nil {
return err
}
if (desc.GetStickyBit() != hlc.Timestamp{}) {
// Swallow "key is not the start of a range" errors because it would mean
// that the sticky bit was removed and merged concurrently. DROP TABLE
// should not fail because of this.
if err := p.ExecCfg().DB.AdminUnsplit(ctx, desc.StartKey); err != nil && !strings.Contains(err.Error(), "is not the start of a range") {
return err
}
}
}
}

Expand Down

0 comments on commit f4cdaab

Please sign in to comment.