diff --git a/build/bazelutil/check.sh b/build/bazelutil/check.sh index 25852a8f1880..768bc9378bd9 100755 --- a/build/bazelutil/check.sh +++ b/build/bazelutil/check.sh @@ -24,6 +24,7 @@ pkg/security/securitytest/securitytest.go://go:generate go-bindata -mode 0600 -m pkg/security/securitytest/securitytest.go://go:generate gofmt -s -w embedded.go pkg/security/securitytest/securitytest.go://go:generate goimports -w embedded.go pkg/server/api_v2.go://go:generate swagger generate spec -w . -o ../../docs/generated/swagger/spec.json --scan-models +pkg/spanconfig/spanconfigstore/span_store.go://go:generate ../../util/interval/generic/gen.sh *entry spanconfigstore pkg/sql/conn_fsm.go://go:generate ../util/fsm/gen/reports.sh TxnStateTransitions stateNoTxn pkg/sql/opt/optgen/lang/gen.go://go:generate langgen -out expr.og.go exprs lang.opt pkg/sql/opt/optgen/lang/gen.go://go:generate langgen -out operator.og.go ops lang.opt diff --git a/pkg/gen/misc.bzl b/pkg/gen/misc.bzl index dbb0df73a5a8..a8a8e8f12324 100644 --- a/pkg/gen/misc.bzl +++ b/pkg/gen/misc.bzl @@ -7,8 +7,8 @@ MISC_SRCS = [ "//pkg/kv/kvserver/spanlatch:latch_interval_btree_test.go", "//pkg/roachpb:batch_generated.go", "//pkg/roachprod/vm/aws:terraform/main.tf", - "//pkg/spanconfig/spanconfigstore:spanconfigstoreentry_interval_btree.go", - "//pkg/spanconfig/spanconfigstore:spanconfigstoreentry_interval_btree_test.go", + "//pkg/spanconfig/spanconfigstore:entry_interval_btree.go", + "//pkg/spanconfig/spanconfigstore:entry_interval_btree_test.go", "//pkg/sql/lexbase:keywords.go", "//pkg/sql/lexbase:reserved_keywords.go", "//pkg/sql/lexbase:tokens.go", diff --git a/pkg/spanconfig/spanconfigreconciler/reconciler.go b/pkg/spanconfig/spanconfigreconciler/reconciler.go index 5f4ea560b035..fee20612ac3f 100644 --- a/pkg/spanconfig/spanconfigreconciler/reconciler.go +++ b/pkg/spanconfig/spanconfigreconciler/reconciler.go @@ -263,7 +263,7 @@ func (f *fullReconciler) reconcile( // KV, in order to delete them. After doing so, we'll issue those same // deletions against this copy in order for it to reflect an up-to-date view // of span configs. - storeWithLatestSpanConfigs = storeWithExistingSpanConfigs.Copy(ctx) + storeWithLatestSpanConfigs = storeWithExistingSpanConfigs.Clone() // Delete all updated spans in a store populated with all current records. // Because our translation above captures the entire SQL state, deleting all diff --git a/pkg/spanconfig/spanconfigstore/BUILD.bazel b/pkg/spanconfig/spanconfigstore/BUILD.bazel index 3801bdcec577..58767e96dc46 100644 --- a/pkg/spanconfig/spanconfigstore/BUILD.bazel +++ b/pkg/spanconfig/spanconfigstore/BUILD.bazel @@ -8,7 +8,7 @@ go_library( "span_store.go", "store.go", "system_store.go", - ":spanconfigstoreentry_interval_btree.go", # keep + ":entry_interval_btree.go", # keep ], importpath = "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigstore", visibility = ["//visibility:public"], @@ -33,7 +33,7 @@ go_test( "span_store_test.go", "store_test.go", "system_store_test.go", - ":spanconfigstoreentry_interval_btree_test.go", # keep + ":entry_interval_btree_test.go", # keep ], data = glob(["testdata/**"]), embed = [":spanconfigstore"], @@ -46,7 +46,6 @@ go_test( "//pkg/testutils", "//pkg/util/hlc", "//pkg/util/leaktest", - "//pkg/util/log", "//pkg/util/randutil", "@com_github_cockroachdb_datadriven//:datadriven", "@com_github_stretchr_testify//require", @@ -56,5 +55,5 @@ go_test( gen_interval_btree( name = "spanconfig_interval_btree", package = "spanconfigstore", - type = "*spanConfigStoreEntry", + type = "*entry", ) diff --git a/pkg/spanconfig/spanconfigstore/spanconfigstoreentry_interval_btree.go b/pkg/spanconfig/spanconfigstore/entry_interval_btree.go similarity index 94% rename from pkg/spanconfig/spanconfigstore/spanconfigstoreentry_interval_btree.go rename to pkg/spanconfig/spanconfigstore/entry_interval_btree.go index 3a932afa1dfb..111824393799 100644 --- a/pkg/spanconfig/spanconfigstore/spanconfigstoreentry_interval_btree.go +++ b/pkg/spanconfig/spanconfigstore/entry_interval_btree.go @@ -22,7 +22,7 @@ import ( ) // nilT is a nil instance of the Template type. -var nilT *spanConfigStoreEntry +var nilT *entry const ( degree = 16 @@ -43,7 +43,7 @@ const ( // c == 0 if (a.Key(), a.EndKey(), a.ID()) == (b.Key(), b.EndKey(), b.ID()) // c == 1 if (a.Key(), a.EndKey(), a.ID()) > (b.Key(), b.EndKey(), b.ID()) // -func cmp(a, b *spanConfigStoreEntry) int { +func cmp(a, b *entry) int { c := bytes.Compare(a.Key(), b.Key()) if c != 0 { return c @@ -81,7 +81,7 @@ func (b keyBound) compare(o keyBound) int { return -1 } -func (b keyBound) contains(a *spanConfigStoreEntry) bool { +func (b keyBound) contains(a *entry) bool { c := bytes.Compare(a.Key(), b.key) if c == 0 { return b.inc @@ -89,7 +89,7 @@ func (b keyBound) contains(a *spanConfigStoreEntry) bool { return c < 0 } -func upperBound(c *spanConfigStoreEntry) keyBound { +func upperBound(c *entry) keyBound { if len(c.EndKey()) != 0 { return keyBound{key: c.EndKey()} } @@ -101,7 +101,7 @@ type leafNode struct { count int16 leaf bool max keyBound - items [maxItems]*spanConfigStoreEntry + items [maxItems]*entry } type node struct { @@ -221,7 +221,7 @@ func (n *node) clone() *node { return c } -func (n *node) insertAt(index int, item *spanConfigStoreEntry, nd *node) { +func (n *node) insertAt(index int, item *entry, nd *node) { if index < int(n.count) { copy(n.items[index+1:n.count+1], n.items[index:n.count]) if !n.leaf { @@ -235,7 +235,7 @@ func (n *node) insertAt(index int, item *spanConfigStoreEntry, nd *node) { n.count++ } -func (n *node) pushBack(item *spanConfigStoreEntry, nd *node) { +func (n *node) pushBack(item *entry, nd *node) { n.items[n.count] = item if !n.leaf { n.children[n.count+1] = nd @@ -243,7 +243,7 @@ func (n *node) pushBack(item *spanConfigStoreEntry, nd *node) { n.count++ } -func (n *node) pushFront(item *spanConfigStoreEntry, nd *node) { +func (n *node) pushFront(item *entry, nd *node) { if !n.leaf { copy(n.children[1:n.count+2], n.children[:n.count+1]) n.children[0] = nd @@ -255,7 +255,7 @@ func (n *node) pushFront(item *spanConfigStoreEntry, nd *node) { // removeAt removes a value at a given index, pulling all subsequent values // back. -func (n *node) removeAt(index int) (*spanConfigStoreEntry, *node) { +func (n *node) removeAt(index int) (*entry, *node) { var child *node if !n.leaf { child = n.children[index+1] @@ -270,7 +270,7 @@ func (n *node) removeAt(index int) (*spanConfigStoreEntry, *node) { } // popBack removes and returns the last element in the list. -func (n *node) popBack() (*spanConfigStoreEntry, *node) { +func (n *node) popBack() (*entry, *node) { n.count-- out := n.items[n.count] n.items[n.count] = nilT @@ -283,7 +283,7 @@ func (n *node) popBack() (*spanConfigStoreEntry, *node) { } // popFront removes and returns the first element in the list. -func (n *node) popFront() (*spanConfigStoreEntry, *node) { +func (n *node) popFront() (*entry, *node) { n.count-- var child *node if !n.leaf { @@ -300,7 +300,7 @@ func (n *node) popFront() (*spanConfigStoreEntry, *node) { // find returns the index where the given item should be inserted into this // list. 'found' is true if the item already exists in the list at the given // index. -func (n *node) find(item *spanConfigStoreEntry) (index int, found bool) { +func (n *node) find(item *entry) (index int, found bool) { // Logic copied from sort.Search. Inlining this gave // an 11% speedup on BenchmarkBTreeDeleteInsert. i, j := 0, int(n.count) @@ -340,7 +340,7 @@ func (n *node) find(item *spanConfigStoreEntry) (index int, found bool) { // | x | | z | // +-----------+ +-----------+ // -func (n *node) split(i int) (*spanConfigStoreEntry, *node) { +func (n *node) split(i int) (*entry, *node) { out := n.items[i] var next *node if n.leaf { @@ -375,7 +375,7 @@ func (n *node) split(i int) (*spanConfigStoreEntry, *node) { // nodes in the subtree exceed maxItems items. Returns true if an existing item // was replaced and false if an item was inserted. Also returns whether the // node's upper bound changes. -func (n *node) insert(item *spanConfigStoreEntry) (replaced, newBound bool) { +func (n *node) insert(item *entry) (replaced, newBound bool) { i, found := n.find(item) if found { n.items[i] = item @@ -408,7 +408,7 @@ func (n *node) insert(item *spanConfigStoreEntry) (replaced, newBound bool) { // removeMax removes and returns the maximum item from the subtree rooted at // this node. -func (n *node) removeMax() *spanConfigStoreEntry { +func (n *node) removeMax() *entry { if n.leaf { n.count-- out := n.items[n.count] @@ -432,7 +432,7 @@ func (n *node) removeMax() *spanConfigStoreEntry { // remove removes an item from the subtree rooted at this node. Returns the item // that was removed or nil if no matching item was found. Also returns whether // the node's upper bound changes. -func (n *node) remove(item *spanConfigStoreEntry) (out *spanConfigStoreEntry, newBound bool) { +func (n *node) remove(item *entry) (out *entry, newBound bool) { i, found := n.find(item) if n.leaf { if found { @@ -609,7 +609,7 @@ func (n *node) findUpperBound() keyBound { // adjustUpperBoundOnInsertion adjusts the upper key bound for this node given // an item and an optional child node that was inserted. Returns true is the // upper bound was changed and false if not. -func (n *node) adjustUpperBoundOnInsertion(item *spanConfigStoreEntry, child *node) bool { +func (n *node) adjustUpperBoundOnInsertion(item *entry, child *node) bool { up := upperBound(item) if child != nil { if up.compare(child.max) < 0 { @@ -626,7 +626,7 @@ func (n *node) adjustUpperBoundOnInsertion(item *spanConfigStoreEntry, child *no // adjustUpperBoundOnRemoval adjusts the upper key bound for this node given an // item and an optional child node that was removed. Returns true is the upper // bound was changed and false if not. -func (n *node) adjustUpperBoundOnRemoval(item *spanConfigStoreEntry, child *node) bool { +func (n *node) adjustUpperBoundOnRemoval(item *entry, child *node) bool { up := upperBound(item) if child != nil { if up.compare(child.max) < 0 { @@ -693,7 +693,7 @@ func (t *btree) Clone() btree { } // Delete removes an item equal to the passed in item from the tree. -func (t *btree) Delete(item *spanConfigStoreEntry) { +func (t *btree) Delete(item *entry) { if t.root == nil || t.root.count == 0 { return } @@ -713,7 +713,7 @@ func (t *btree) Delete(item *spanConfigStoreEntry) { // Set adds the given item to the tree. If an item in the tree already equals // the given one, it is replaced with the new item. -func (t *btree) Set(item *spanConfigStoreEntry) { +func (t *btree) Set(item *entry) { if t.root == nil { t.root = newLeafNode() } else if t.root.count >= maxItems { @@ -875,7 +875,7 @@ func (i *iterator) ascend() { // SeekGE seeks to the first item greater-than or equal to the provided // item. -func (i *iterator) SeekGE(item *spanConfigStoreEntry) { +func (i *iterator) SeekGE(item *entry) { i.reset() if i.n == nil { return @@ -897,7 +897,7 @@ func (i *iterator) SeekGE(item *spanConfigStoreEntry) { } // SeekLT seeks to the first item less-than the provided item. -func (i *iterator) SeekLT(item *spanConfigStoreEntry) { +func (i *iterator) SeekLT(item *entry) { i.reset() if i.n == nil { return @@ -995,7 +995,7 @@ func (i *iterator) Valid() bool { // Cur returns the item at the iterator's current position. It is illegal // to call Cur if the iterator is not valid. -func (i *iterator) Cur() *spanConfigStoreEntry { +func (i *iterator) Cur() *entry { return i.n.items[i.pos] } @@ -1056,7 +1056,7 @@ type overlapScan struct { // FirstOverlap seeks to the first item in the btree that overlaps with the // provided search item. -func (i *iterator) FirstOverlap(item *spanConfigStoreEntry) { +func (i *iterator) FirstOverlap(item *entry) { i.reset() if i.n == nil { return @@ -1070,7 +1070,7 @@ func (i *iterator) FirstOverlap(item *spanConfigStoreEntry) { // NextOverlap positions the iterator to the item immediately following // its current position that overlaps with the search item. -func (i *iterator) NextOverlap(item *spanConfigStoreEntry) { +func (i *iterator) NextOverlap(item *entry) { if i.n == nil { return } @@ -1078,7 +1078,7 @@ func (i *iterator) NextOverlap(item *spanConfigStoreEntry) { i.findNextOverlap(item) } -func (i *iterator) constrainMinSearchBounds(item *spanConfigStoreEntry) { +func (i *iterator) constrainMinSearchBounds(item *entry) { k := item.Key() j := sort.Search(int(i.n.count), func(j int) bool { return bytes.Compare(k, i.n.items[j].Key()) <= 0 @@ -1087,7 +1087,7 @@ func (i *iterator) constrainMinSearchBounds(item *spanConfigStoreEntry) { i.o.constrMinPos = int16(j) } -func (i *iterator) constrainMaxSearchBounds(item *spanConfigStoreEntry) { +func (i *iterator) constrainMaxSearchBounds(item *entry) { up := upperBound(item) j := sort.Search(int(i.n.count), func(j int) bool { return !up.contains(i.n.items[j]) @@ -1096,7 +1096,7 @@ func (i *iterator) constrainMaxSearchBounds(item *spanConfigStoreEntry) { i.o.constrMaxPos = int16(j) } -func (i *iterator) findNextOverlap(item *spanConfigStoreEntry) { +func (i *iterator) findNextOverlap(item *entry) { for { if i.pos > i.n.count { // Iterate up tree. diff --git a/pkg/spanconfig/spanconfigstore/spanconfigstoreentry_interval_btree_test.go b/pkg/spanconfig/spanconfigstore/entry_interval_btree_test.go similarity index 98% rename from pkg/spanconfig/spanconfigstore/spanconfigstoreentry_interval_btree_test.go rename to pkg/spanconfig/spanconfigstore/entry_interval_btree_test.go index 152c35f08ab5..77f7623784f8 100644 --- a/pkg/spanconfig/spanconfigstore/spanconfigstoreentry_interval_btree_test.go +++ b/pkg/spanconfig/spanconfigstore/entry_interval_btree_test.go @@ -26,14 +26,14 @@ import ( "github.com/stretchr/testify/require" ) -func newItem(s roachpb.Span) *spanConfigStoreEntry { +func newItem(s roachpb.Span) *entry { i := nilT.New() i.SetKey(s.Key) i.SetEndKey(s.EndKey) return i } -func spanFromItem(i *spanConfigStoreEntry) roachpb.Span { +func spanFromItem(i *entry) roachpb.Span { return roachpb.Span{Key: i.Key(), EndKey: i.EndKey()} } @@ -515,7 +515,7 @@ func TestIterStack(t *testing.T) { ////////////////////////////////////////// // perm returns a random permutation of items with spans in the range [0, n). -func perm(n int) (out []*spanConfigStoreEntry) { +func perm(n int) (out []*entry) { for _, i := range rand.Perm(n) { out = append(out, newItem(spanWithEnd(i, i+1))) } @@ -523,7 +523,7 @@ func perm(n int) (out []*spanConfigStoreEntry) { } // rang returns an ordered list of items with spans in the range [m, n]. -func rang(m, n int) (out []*spanConfigStoreEntry) { +func rang(m, n int) (out []*entry) { for i := m; i <= n; i++ { out = append(out, newItem(spanWithEnd(i, i+1))) } @@ -531,7 +531,7 @@ func rang(m, n int) (out []*spanConfigStoreEntry) { } // all extracts all items from a tree in order as a slice. -func all(tr *btree) (out []*spanConfigStoreEntry) { +func all(tr *btree) (out []*entry) { it := tr.MakeIter() it.First() for it.Valid() { @@ -743,10 +743,10 @@ func TestBTreeSeekOverlapRandom(t *testing.T) { var tr btree const count = 1000 - items := make([]*spanConfigStoreEntry, count) + items := make([]*entry, count) itemSpans := make([]int, count) for j := 0; j < count; j++ { - var item *spanConfigStoreEntry + var item *entry end := rng.Intn(count + 10) if end <= j { end = j @@ -761,7 +761,7 @@ func TestBTreeSeekOverlapRandom(t *testing.T) { const scanTrials = 100 for j := 0; j < scanTrials; j++ { - var scanItem *spanConfigStoreEntry + var scanItem *entry scanStart := rng.Intn(count) scanEnd := rng.Intn(count + 10) if scanEnd <= scanStart { @@ -771,7 +771,7 @@ func TestBTreeSeekOverlapRandom(t *testing.T) { scanItem = newItem(spanWithEnd(scanStart, scanEnd+1)) } - var exp, found []*spanConfigStoreEntry + var exp, found []*entry for startKey, endKey := range itemSpans { if startKey <= scanEnd && endKey >= scanStart { exp = append(exp, items[startKey]) @@ -853,7 +853,7 @@ func TestBTreeCloneConcurrentOperations(t *testing.T) { t.Log("Checking all values again") for i, tree := range trees { - var wantpart []*spanConfigStoreEntry + var wantpart []*entry if i < len(trees)/2 { wantpart = want[:cloneTestSize/2] } else { diff --git a/pkg/spanconfig/spanconfigstore/interner.go b/pkg/spanconfig/spanconfigstore/interner.go index 55a1d5646cba..c389b3f7c3c7 100644 --- a/pkg/spanconfig/spanconfigstore/interner.go +++ b/pkg/spanconfig/spanconfigstore/interner.go @@ -18,71 +18,66 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/protoutil" ) -type internerID uint64 +type spanConfigKey string // interner interns span config protos. It's a ref-counted data structure that // maintains a single copy of every unique config that that's been added to -// it. Configs that are maintained are identified/retrivable using an -// internerID. Configs can also be removed, and if there are no more references -// to it, we no longer hold onto it in memory. +// it. Configs can also be removed, and if there are no more references to it, +// we no longer hold onto it in memory. type interner struct { - internIDAlloc internerID - - configToID map[string]internerID - idToConfig map[internerID]roachpb.SpanConfig - idToRefCount map[internerID]uint64 + configToCanonical map[spanConfigKey]*roachpb.SpanConfig + refCounts map[*roachpb.SpanConfig]uint64 } func newInterner() *interner { return &interner{ - configToID: make(map[string]internerID), - idToConfig: make(map[internerID]roachpb.SpanConfig), - idToRefCount: make(map[internerID]uint64), + configToCanonical: make(map[spanConfigKey]*roachpb.SpanConfig), + refCounts: make(map[*roachpb.SpanConfig]uint64), } } -func (i *interner) add(ctx context.Context, conf roachpb.SpanConfig) internerID { +func (i *interner) add(ctx context.Context, conf roachpb.SpanConfig) *roachpb.SpanConfig { marshalled, err := protoutil.Marshal(&conf) if err != nil { log.Fatalf(ctx, "%v", err) } - if id, found := i.configToID[string(marshalled)]; found { - i.idToRefCount[id]++ - return id + if canonical, found := i.configToCanonical[spanConfigKey(marshalled)]; found { + i.refCounts[canonical]++ + return canonical } - i.internIDAlloc++ - - id := i.internIDAlloc - i.configToID[string(marshalled)] = id - i.idToConfig[i.internIDAlloc] = conf - i.idToRefCount[id] = 1 - return id + i.configToCanonical[spanConfigKey(marshalled)] = &conf + i.refCounts[&conf] = 1 + return &conf } -func (i *interner) get(id internerID) (roachpb.SpanConfig, bool) { - conf, found := i.idToConfig[id] - return conf, found -} - -func (i *interner) remove(ctx context.Context, id internerID) { - conf, found := i.idToConfig[id] - if !found { +func (i *interner) remove(ctx context.Context, conf *roachpb.SpanConfig) { + if _, found := i.refCounts[conf]; !found { return // nothing to do } - i.idToRefCount[id]-- - if i.idToRefCount[id] != 0 { + i.refCounts[conf]-- + if i.refCounts[conf] != 0 { return // nothing to do } - marshalled, err := protoutil.Marshal(&conf) + marshalled, err := protoutil.Marshal(conf) if err != nil { - log.Infof(ctx, "%v", err) + log.Fatalf(ctx, "%v", err) } - delete(i.idToConfig, id) - delete(i.idToRefCount, id) - delete(i.configToID, string(marshalled)) + delete(i.refCounts, conf) + delete(i.configToCanonical, spanConfigKey(marshalled)) +} + +func (i *interner) copy() *interner { + copiedInterner := newInterner() + for k, v := range i.configToCanonical { + copiedInterner.configToCanonical[k] = v + } + for k, v := range i.refCounts { + copiedInterner.refCounts[k] = v + } + return copiedInterner } diff --git a/pkg/spanconfig/spanconfigstore/span_store.go b/pkg/spanconfig/spanconfigstore/span_store.go index b6767f03791c..bfc07d14759c 100644 --- a/pkg/spanconfig/spanconfigstore/span_store.go +++ b/pkg/spanconfig/spanconfigstore/span_store.go @@ -45,13 +45,13 @@ var hostCoalesceAdjacentSetting = settings.RegisterBoolSetting( ) // spanConfigStore is an in-memory data structure to store and retrieve -// SpanConfigs associated with a single span. Internally it makes use of an -// interval tree to store non-overlapping span configurations. It isn't safe for +// SpanConfigs associated with a single span. Internally it makes use of a +// b-tree to store non-overlapping span configurations. It isn't safe for // concurrent use. type spanConfigStore struct { btree *btree - treeIDAlloc uint64 - interner *interner + treeIDAlloc uint64 // used to maintain unique IDs for entries in btree + interner *interner // interns configs for fast comparison settings *cluster.Settings knobs *spanconfig.TestingKnobs @@ -74,20 +74,14 @@ func newSpanConfigStore( } // copy returns a copy of the spanConfigStore. -func (s *spanConfigStore) copy(ctx context.Context) *spanConfigStore { - clone := newSpanConfigStore(s.settings, s.knobs) - _ = s.forEachOverlapping(keys.EverythingSpan, func(sp roachpb.Span, conf roachpb.SpanConfig) error { - record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(sp), conf) - if err != nil { - log.Fatalf(ctx, "%v", err) - } - _, _, err = clone.apply(ctx, false /* dryrun */, spanconfig.Update(record)) - if err != nil { - log.Fatalf(ctx, "%v", err) - } - return nil - }) - return clone +func (s *spanConfigStore) clone() *spanConfigStore { + clonedTree := s.btree.Clone() + return &spanConfigStore{ + settings: s.settings, + knobs: s.knobs, + btree: &clonedTree, + interner: s.interner.copy(), + } } // forEachOverlapping iterates through the set of entries that overlap with the @@ -100,8 +94,8 @@ func (s *spanConfigStore) forEachOverlapping( // corresponding span config entries. iter, query := s.btree.MakeIter(), makeQueryEntry(sp) for iter.FirstOverlap(query); iter.Valid(); iter.NextOverlap(query) { - entry := iter.Cur().spanConfigPairInterned - if err := f(entry.span, entry.conf(s.interner)); err != nil { + interned := iter.Cur().spanConfigPairInterned + if err := f(interned.span, interned.conf()); err != nil { if iterutil.Done(err) { err = nil } @@ -113,9 +107,7 @@ func (s *spanConfigStore) forEachOverlapping( // computeSplitKey returns the first key we should split on because of the // presence a span config given a start and end key pair. -func (s *spanConfigStore) computeSplitKey( - ctx context.Context, start, end roachpb.RKey, -) roachpb.RKey { +func (s *spanConfigStore) computeSplitKey(start, end roachpb.RKey) (roachpb.RKey, error) { sp := roachpb.Span{Key: start.AsRawKey(), EndKey: end.AsRawKey()} // We don't want to split within the system config span while we're still @@ -124,10 +116,10 @@ func (s *spanConfigStore) computeSplitKey( // TODO(irfansharif): Once we've fully phased out the system config span, we // can get rid of this special handling. if keys.SystemConfigSpan.Contains(sp) { - return nil + return nil, nil } if keys.SystemConfigSpan.ContainsKey(sp.Key) { - return roachpb.RKey(keys.SystemConfigSpan.EndKey) + return roachpb.RKey(keys.SystemConfigSpan.EndKey), nil } // Generally split keys are going to be the start keys of span config entries. @@ -139,38 +131,48 @@ func (s *spanConfigStore) computeSplitKey( { iter, query := s.btree.MakeIter(), makeQueryEntry(sp) for iter.FirstOverlap(query); iter.Valid(); iter.NextOverlap(query) { - entry := iter.Cur().spanConfigPairInterned - if entry.span.Key.Compare(sp.Key) <= 0 { + interned := iter.Cur().spanConfigPairInterned + if interned.span.Key.Compare(sp.Key) <= 0 { continue // more } - match = entry + match = interned break // we found our split key, we're done } if match.isEmpty() { - return nil // no overlapping entries == no split key + return nil, nil // no overlapping entries == no split key } } if s.knobs.StoreDisableCoalesceAdjacent { - return roachpb.RKey(match.span.Key) + return roachpb.RKey(match.span.Key), nil } rem, matchTenID, err := keys.DecodeTenantPrefix(match.span.Key) if err != nil { - log.Fatalf(ctx, "%v", err) + return nil, err } if !s.knobs.StoreIgnoreCoalesceAdjacentExceptions { if matchTenID.IsSystem() { + // NB: This MaxReservedDescID fellow isn't really that meaningful anymore, + // it's just the maximum ID system tables could have had before dynamic + // system table IDs were thing. This check here is out of an abundance of + // caution, for these older system tables there's little benefit to + // coalescing them even though we can: it lowers the range count slightly + // but at the risk of subsystems stepping on one another by sharing these + // ranges. That said, we take no such precautions in the tenant keyspace, + // so perhaps it's overblown. Perhaps we want general config attributes + // that opt a specific table/index out of being coalesced with adjacent + // ranges. systemTableUpperBound := keys.SystemSQLCodec.TablePrefix(keys.MaxReservedDescID + 1) if roachpb.Key(rem).Compare(systemTableUpperBound) < 0 || !hostCoalesceAdjacentSetting.Get(&s.settings.SV) { - return roachpb.RKey(match.span.Key) + return roachpb.RKey(match.span.Key), nil } } else { if !tenantCoalesceAdjacentSetting.Get(&s.settings.SV) { - return roachpb.RKey(match.span.Key) + return roachpb.RKey(match.span.Key), nil } } } @@ -186,19 +188,18 @@ func (s *spanConfigStore) computeSplitKey( preSplitKeySp := roachpb.Span{Key: sp.Key, EndKey: match.span.Key} { iter, query := s.btree.MakeIter(), makeQueryEntry(preSplitKeySp) - for iter.FirstOverlap(query); iter.Valid(); { + if iter.FirstOverlap(query); iter.Valid() { firstMatch = iter.Cur().spanConfigPairInterned - break // we're done } if firstMatch.isEmpty() { - return roachpb.RKey(match.span.Key) + return roachpb.RKey(match.span.Key), nil } _, firstMatchTenID, err := keys.DecodeTenantPrefix(firstMatch.span.Key) if err != nil { - log.Fatalf(ctx, "%v", err) + return nil, err } - if firstMatch.internerID != match.internerID || firstMatchTenID.ToUint64() != matchTenID.ToUint64() { - return roachpb.RKey(match.span.Key) + if firstMatch.canonical != match.canonical || firstMatchTenID.ToUint64() != matchTenID.ToUint64() { + return roachpb.RKey(match.span.Key), nil } } @@ -213,21 +214,21 @@ func (s *spanConfigStore) computeSplitKey( nextEntry := iter.Cur().spanConfigPairInterned _, entryTenID, err := keys.DecodeTenantPrefix(nextEntry.span.Key) if err != nil { - log.Fatalf(ctx, "%v", err) + return nil, err } - if nextEntry.internerID != match.internerID || entryTenID.ToUint64() != matchTenID.ToUint64() { + if nextEntry.canonical != match.canonical || entryTenID.ToUint64() != matchTenID.ToUint64() { lastMatch = nextEntry break // we're done } } if !lastMatch.isEmpty() { - return roachpb.RKey(lastMatch.span.Key) + return roachpb.RKey(lastMatch.span.Key), nil } } // All entries within the given span have the same config and part of the same // tenant. There are no split points here. - return nil + return nil, nil } // getSpanConfigForKey returns the span config corresponding to the supplied @@ -238,7 +239,7 @@ func (s *spanConfigStore) getSpanConfigForKey( sp := roachpb.Span{Key: key.AsRawKey(), EndKey: key.Next().AsRawKey()} iter, query := s.btree.MakeIter(), makeQueryEntry(sp) for iter.FirstOverlap(query); iter.Valid(); { - conf, found = iter.Cur().conf(s.interner), true + conf, found = iter.Cur().conf(), true break } if !found && log.ExpensiveLogEnabled(ctx, 1) { @@ -252,7 +253,7 @@ func (s *spanConfigStore) getSpanConfigForKey( // by applying them if dryrun is false. func (s *spanConfigStore) apply( ctx context.Context, dryrun bool, updates ...spanconfig.Update, -) (deleted []roachpb.Span, added []spanConfigStoreEntry, err error) { +) (deleted []roachpb.Span, added []entry, err error) { if err := validateApplyArgs(updates...); err != nil { return nil, nil, err } @@ -274,12 +275,12 @@ func (s *spanConfigStore) apply( entry := &entriesToDelete[i] if !dryrun { s.btree.Delete(entry) - s.interner.remove(ctx, entry.internerID) + s.interner.remove(ctx, entry.canonical) } deleted[i] = entry.span } - added = make([]spanConfigStoreEntry, len(entriesToAdd)) + added = make([]entry, len(entriesToAdd)) for i := range entriesToAdd { entry := &entriesToAdd[i] if !dryrun { @@ -387,7 +388,7 @@ func (s *spanConfigStore) apply( // func (s *spanConfigStore) accumulateOpsFor( ctx context.Context, dryrun bool, updates []spanconfig.Update, -) (toDelete, toAdd []spanConfigStoreEntry, _ error) { +) (toDelete, toAdd []entry, _ error) { var carryOver spanConfigPair for _, update := range updates { var carriedOver spanConfigPair @@ -429,7 +430,7 @@ func (s *spanConfigStore) accumulateOpsFor( post = roachpb.Span{Key: inter.EndKey, EndKey: union.EndKey} ) - existingConf := existing.conf(s.interner) + existingConf := existing.conf() if update.Addition() { if existing.span.Equal(update.GetTarget().GetSpan()) && existingConf.Equal(update.GetConfig()) { skipAddingSelf = true @@ -548,59 +549,58 @@ func (s *spanConfigPair) isEmpty() bool { // spanConfigPair, doesn't embed the span config itself. It instead holds onto // an interner ID which can be used to retrieve the corresponding config. type spanConfigPairInterned struct { - span roachpb.Span - internerID + span roachpb.Span + canonical *roachpb.SpanConfig } func (s *spanConfigPairInterned) isEmpty() bool { - return s.span.Equal(roachpb.Span{}) && s.internerID == internerID(0) + return s.span.Equal(roachpb.Span{}) && s.canonical == nil } -func (s *spanConfigPairInterned) conf(interner *interner) roachpb.SpanConfig { - conf, _ := interner.get(s.internerID) - return conf +func (s *spanConfigPairInterned) conf() roachpb.SpanConfig { + return *s.canonical } -//go:generate ../../util/interval/generic/gen.sh *spanConfigStoreEntry spanconfigstore +//go:generate ../../util/interval/generic/gen.sh *entry spanconfigstore -type spanConfigStoreEntry struct { +type entry struct { spanConfigPairInterned id uint64 } func (s *spanConfigStore) makeEntry( ctx context.Context, dryrun bool, sp roachpb.Span, conf roachpb.SpanConfig, -) spanConfigStoreEntry { +) entry { if !dryrun { s.treeIDAlloc++ } - var internID internerID + var canonical *roachpb.SpanConfig if !dryrun || s.knobs.StoreInternConfigsInDryRuns { - internID = s.interner.add(ctx, conf) + canonical = s.interner.add(ctx, conf) } - return spanConfigStoreEntry{ + return entry{ spanConfigPairInterned: spanConfigPairInterned{ - span: sp, - internerID: internID, + span: sp, + canonical: canonical, }, id: s.treeIDAlloc, } } -func makeQueryEntry(s roachpb.Span) *spanConfigStoreEntry { - var entry spanConfigStoreEntry - entry.SetKey(s.Key) - entry.SetEndKey(s.EndKey) - return &entry +func makeQueryEntry(s roachpb.Span) *entry { + var e entry + e.SetKey(s.Key) + e.SetEndKey(s.EndKey) + return &e } // Methods required by util/interval/generic type contract. -func (s *spanConfigStoreEntry) ID() uint64 { return s.id } -func (s *spanConfigStoreEntry) Key() []byte { return s.span.Key } -func (s *spanConfigStoreEntry) EndKey() []byte { return s.span.EndKey } -func (s *spanConfigStoreEntry) String() string { return s.span.String() } -func (s *spanConfigStoreEntry) New() *spanConfigStoreEntry { return new(spanConfigStoreEntry) } -func (s *spanConfigStoreEntry) SetID(id uint64) { s.id = id } -func (s *spanConfigStoreEntry) SetKey(k []byte) { s.span.Key = k } -func (s *spanConfigStoreEntry) SetEndKey(k []byte) { s.span.EndKey = k } +func (s *entry) ID() uint64 { return s.id } +func (s *entry) Key() []byte { return s.span.Key } +func (s *entry) EndKey() []byte { return s.span.EndKey } +func (s *entry) String() string { return s.span.String() } +func (s *entry) New() *entry { return new(entry) } +func (s *entry) SetID(id uint64) { s.id = id } +func (s *entry) SetKey(k []byte) { s.span.Key = k } +func (s *entry) SetEndKey(k []byte) { s.span.EndKey = k } diff --git a/pkg/spanconfig/spanconfigstore/span_store_test.go b/pkg/spanconfig/spanconfigstore/span_store_test.go index f1f1ae3d07fd..fa8e53bf3494 100644 --- a/pkg/spanconfig/spanconfigstore/span_store_test.go +++ b/pkg/spanconfig/spanconfigstore/span_store_test.go @@ -22,7 +22,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/spanconfig" "github.com/cockroachdb/cockroach/pkg/spanconfig/spanconfigtestutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" - "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/randutil" "github.com/stretchr/testify/require" ) @@ -133,175 +132,201 @@ func TestRandomized(t *testing.T) { } if !expFound { - _ = store.forEachOverlapping(testSpan, - func(sp roachpb.Span, conf roachpb.SpanConfig) error { - record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(sp), conf) - require.NoError(t, err) - t.Fatalf("found unexpected entry: %s", - spanconfigtestutils.PrintSpanConfigRecord(t, record)) - return nil - }, - ) - } else { - var foundSpanConfigPair spanConfigPair - _ = store.forEachOverlapping(testSpan, - func(sp roachpb.Span, conf roachpb.SpanConfig) error { - if !foundSpanConfigPair.isEmpty() { + t.Run("deleted-entry-should-not-appear", func(t *testing.T) { + _ = store.forEachOverlapping(testSpan, + func(sp roachpb.Span, conf roachpb.SpanConfig) error { record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(sp), conf) require.NoError(t, err) - t.Fatalf("expected single overlapping entry, found second: %s", + t.Fatalf("found unexpected entry: %s", spanconfigtestutils.PrintSpanConfigRecord(t, record)) - } - foundSpanConfigPair = spanConfigPair{ - span: sp, - config: conf, - } - - // Check that the entry is exactly what we'd expect. - gotSpan, gotConfig := sp, conf - require.Truef(t, gotSpan.Contains(testSpan), - "improper result: expected retrieved span (%s) to contain test span (%s)", - spanconfigtestutils.PrintSpan(gotSpan), spanconfigtestutils.PrintSpan(testSpan)) - - require.Truef(t, expConfig.Equal(gotConfig), - "mismatched configs: expected %s, got %s", - spanconfigtestutils.PrintSpanConfig(expConfig), spanconfigtestutils.PrintSpanConfig(gotConfig)) - - return nil - }, - ) + return nil + }, + ) + }) + } else { + t.Run("should-observe-last-write", func(t *testing.T) { + var foundSpanConfigPair spanConfigPair + _ = store.forEachOverlapping(testSpan, + func(sp roachpb.Span, conf roachpb.SpanConfig) error { + if !foundSpanConfigPair.isEmpty() { + record, err := spanconfig.MakeRecord(spanconfig.MakeTargetFromSpan(sp), conf) + require.NoError(t, err) + t.Fatalf("expected single overlapping entry, found second: %s", + spanconfigtestutils.PrintSpanConfigRecord(t, record)) + } + foundSpanConfigPair = spanConfigPair{ + span: sp, + config: conf, + } + + // Check that the entry is exactly what we'd expect. + gotSpan, gotConfig := sp, conf + require.Truef(t, gotSpan.Contains(testSpan), + "improper result: expected retrieved span (%s) to contain test span (%s)", + spanconfigtestutils.PrintSpan(gotSpan), spanconfigtestutils.PrintSpan(testSpan)) + + require.Truef(t, expConfig.Equal(gotConfig), + "mismatched configs: expected %s, got %s", + spanconfigtestutils.PrintSpanConfig(expConfig), spanconfigtestutils.PrintSpanConfig(gotConfig)) + + return nil + }, + ) - // Ensure that the config accessed through the StoreReader interface is - // the same as above. - storeReaderConfig, found := store.getSpanConfigForKey(ctx, roachpb.RKey(testSpan.Key)) - require.True(t, found) - require.True(t, foundSpanConfigPair.config.Equal(storeReaderConfig)) + // Ensure that the config accessed through the StoreReader interface is + // the same as above. + storeReaderConfig, found := store.getSpanConfigForKey(ctx, roachpb.RKey(testSpan.Key)) + require.True(t, found) + require.True(t, foundSpanConfigPair.config.Equal(storeReaderConfig)) + }) } - everythingSpan := spanconfigtestutils.ParseSpan(t, fmt.Sprintf("[%s,%s)", - string(alphabet[0]), string(alphabet[len(alphabet)-1]))) + t.Run("entries-are-valid-and-non-overlapping", func(t *testing.T) { + everythingSpan := spanconfigtestutils.ParseSpan(t, fmt.Sprintf("[%s,%s)", + string(alphabet[0]), string(alphabet[len(alphabet)-1]))) + var lastOverlapping spanConfigPair + require.NoError(t, store.forEachOverlapping(everythingSpan, + func(sp roachpb.Span, conf roachpb.SpanConfig) error { + // All spans are expected to be valid. + require.True(t, sp.Valid(), + "expected to only find valid spans, found %s", + spanconfigtestutils.PrintSpan(sp), + ) - var lastOverlapping spanConfigPair - require.NoError(t, store.forEachOverlapping(everythingSpan, - func(sp roachpb.Span, conf roachpb.SpanConfig) error { - log.Infof(ctx, "set %s:%s", spanconfigtestutils.PrintSpan(sp), spanconfigtestutils.PrintSpanConfig(conf)) + if !lastOverlapping.isEmpty() { + // Span configs are returned in strictly sorted order. + require.True(t, lastOverlapping.span.Key.Compare(sp.Key) < 0, + "expected to find spans in strictly sorted order, found %s then %s", + spanconfigtestutils.PrintSpan(lastOverlapping.span), spanconfigtestutils.PrintSpan(sp)) - // All spans are expected to be valid. - require.True(t, sp.Valid(), - "expected to only find valid spans, found %s", - spanconfigtestutils.PrintSpan(sp), - ) + // Span configs must also be non-overlapping. + require.Falsef(t, lastOverlapping.span.Overlaps(sp), + "expected non-overlapping spans, found %s and %s", + spanconfigtestutils.PrintSpan(lastOverlapping.span), spanconfigtestutils.PrintSpan(sp)) + } - if !lastOverlapping.isEmpty() { - // Span configs are returned in strictly sorted order. - require.True(t, lastOverlapping.span.Key.Compare(sp.Key) < 0, - "expected to find spans in strictly sorted order, found %s then %s", - spanconfigtestutils.PrintSpan(lastOverlapping.span), spanconfigtestutils.PrintSpan(sp)) + lastOverlapping = spanConfigPair{ + span: sp, + config: conf, + } + return nil + }, + )) + }) - // Span configs must also be non-overlapping. - require.Falsef(t, lastOverlapping.span.Overlaps(sp), - "expected non-overlapping spans, found %s and %s", - spanconfigtestutils.PrintSpan(lastOverlapping.span), spanconfigtestutils.PrintSpan(sp)) - } + t.Run("split-key-properties", func(t *testing.T) { + // Properties for computed split keys: + // + // (1) sp.ProperlyContains(ComputeSplitKey(sp)) + // (i) When computing the set of all split keys over a given span, they + // should be sorted + // (2) Configs at adjacent split keys must be non-identical + // (3) Span config entries between two split keys 'a' and 'b' should have a + // config identical to the config at 'a' + // (i) The config at the last split key should be identical the last + // config that overlaps with the query span + // (ii) Properties (3) and (1) imply that the first split key should + // have a different config than the first config that overlaps with + // the query span if the first entry start key == query span start + // key. + // + // This subtest computes the set of all split keys over a randomly generated + // span and asserts on all the properties above. + querySpan := getRandomSpan() + splitKeys := store.TestingSplitKeys(t, + roachpb.RKey(querySpan.Key), + roachpb.RKey(querySpan.EndKey), + ) - lastOverlapping = spanConfigPair{ - span: sp, - config: conf, - } - return nil - }, - )) - - querySpan := getRandomSpan() - splitKeys := store.TestingSplitKeys(ctx, - roachpb.RKey(querySpan.Key), - roachpb.RKey(querySpan.EndKey), - ) - - numOverlappingWithQuerySp := 0 - var firstOverlappingWithQuerySp, lastOverlappingWithQuerySp spanConfigPair - require.NoError(t, store.forEachOverlapping(querySpan, - func(sp roachpb.Span, conf roachpb.SpanConfig) error { - if numOverlappingWithQuerySp == 0 { - firstOverlappingWithQuerySp = spanConfigPair{ + numOverlappingWithQuerySp := 0 + var firstOverlappingWithQuerySp, lastOverlappingWithQuerySp spanConfigPair + require.NoError(t, store.forEachOverlapping(querySpan, + func(sp roachpb.Span, conf roachpb.SpanConfig) error { + if numOverlappingWithQuerySp == 0 { + firstOverlappingWithQuerySp = spanConfigPair{ + span: sp, + config: conf, + } + } + numOverlappingWithQuerySp++ + lastOverlappingWithQuerySp = spanConfigPair{ span: sp, config: conf, } - } - numOverlappingWithQuerySp++ - lastOverlappingWithQuerySp = spanConfigPair{ - span: sp, - config: conf, - } - return nil - }, - )) - - var lastSplitKey roachpb.RKey - var confAtLastSplitKey roachpb.SpanConfig - for i, curSplitKey := range splitKeys { - require.Truef(t, querySpan.ProperlyContainsKey(curSplitKey.AsRawKey()), - "invalid split key %s (over span %s)", curSplitKey, querySpan) - - confAtCurSplitKey, found := store.getSpanConfigForKey(ctx, curSplitKey) - require.True(t, found) - - if i == 0 { - require.True(t, firstOverlappingWithQuerySp.span.Key.Compare(curSplitKey.AsRawKey()) <= 0, - "expected to find %s sorted before %s", - firstOverlappingWithQuerySp.span.Key, curSplitKey) - - // The config at the first split key must not be identical to the first - // overlapping span's if the first overlapping span key is the same as the - // makeQueryEntry start key. - if firstOverlappingWithQuerySp.span.Key.Equal(querySpan.Key) { - require.Falsef(t, confAtCurSplitKey.Equal(firstOverlappingWithQuerySp.config), + return nil + }, + )) + + var lastSplitKey roachpb.RKey + var confAtLastSplitKey roachpb.SpanConfig + for i, curSplitKey := range splitKeys { + // Property (1). + require.Truef(t, querySpan.ProperlyContainsKey(curSplitKey.AsRawKey()), + "invalid split key %s (over span %s)", curSplitKey, querySpan) + + confAtCurSplitKey, found := store.getSpanConfigForKey(ctx, curSplitKey) + require.True(t, found) + + if i == 0 { + // Property (1.i). + require.True(t, firstOverlappingWithQuerySp.span.Key.Compare(curSplitKey.AsRawKey()) <= 0, + "expected to find %s sorted before %s", + firstOverlappingWithQuerySp.span.Key, curSplitKey) + + // The config at the first split key must not be identical to the first + // overlapping span's if the first overlapping span key is the same as the + // query start key, property (3.ii). + if firstOverlappingWithQuerySp.span.Key.Equal(querySpan.Key) { + require.Falsef(t, confAtCurSplitKey.Equal(firstOverlappingWithQuerySp.config), + "expected non-identical configs, found %s:%s and %s:%s", + curSplitKey, spanconfigtestutils.PrintSpanConfig(confAtCurSplitKey), + firstOverlappingWithQuerySp.span.Key, spanconfigtestutils.PrintSpanConfig(firstOverlappingWithQuerySp.config), + ) + } + } else { + // Split keys are returned in strictly sorted order, property (1.i). + require.True(t, lastSplitKey.Compare(curSplitKey) < 0, + "expected to find split keys in strictly sorted order, found %s then %s", + lastSplitKey, curSplitKey) + + // Adjacent split key configs must have non-identical configs, property + // (2). + require.Falsef(t, confAtLastSplitKey.Equal(confAtCurSplitKey), "expected non-identical configs, found %s:%s and %s:%s", + lastSplitKey, spanconfigtestutils.PrintSpanConfig(confAtLastSplitKey), curSplitKey, spanconfigtestutils.PrintSpanConfig(confAtCurSplitKey), - firstOverlappingWithQuerySp.span.Key, spanconfigtestutils.PrintSpanConfig(firstOverlappingWithQuerySp.config), ) + + // Span config entries between the split keys should be identical to the + // config at the last split key, property (3). + require.NoError(t, store.forEachOverlapping(roachpb.Span{ + Key: lastSplitKey.AsRawKey(), + EndKey: curSplitKey.AsRawKey(), + }, func(sp roachpb.Span, conf roachpb.SpanConfig) error { + require.Truef(t, confAtLastSplitKey.Equal(conf), + "expected identical configs, found %s:%s and %s:%s", + lastSplitKey, spanconfigtestutils.PrintSpanConfig(confAtLastSplitKey), + sp.Key, spanconfigtestutils.PrintSpanConfig(conf), + ) + return nil + })) } - } else { - // Split keys are returned in strictly sorted order. - require.True(t, lastSplitKey.Compare(curSplitKey) < 0, - "expected to find split keys in strictly sorted order, found %s then %s", - lastSplitKey, curSplitKey) - - // Adjacent split key configs must have non-identical configs. - require.Falsef(t, confAtLastSplitKey.Equal(confAtCurSplitKey), - "expected non-identical configs, found %s:%s and %s:%s", - lastSplitKey, spanconfigtestutils.PrintSpanConfig(confAtLastSplitKey), - curSplitKey, spanconfigtestutils.PrintSpanConfig(confAtCurSplitKey), - ) - // Span config entries between the split keys should be identical to the - // config at the last split key. - require.NoError(t, store.forEachOverlapping(roachpb.Span{ - Key: lastSplitKey.AsRawKey(), - EndKey: curSplitKey.AsRawKey(), - }, func(sp roachpb.Span, conf roachpb.SpanConfig) error { - require.Truef(t, confAtLastSplitKey.Equal(conf), - "expected identical configs, found %s:%s and %s:%s", - lastSplitKey, spanconfigtestutils.PrintSpanConfig(confAtLastSplitKey), - sp.Key, spanconfigtestutils.PrintSpanConfig(conf), - ) - return nil - })) + lastSplitKey, confAtLastSplitKey = curSplitKey, confAtCurSplitKey } - lastSplitKey, confAtLastSplitKey = curSplitKey, confAtCurSplitKey - } - - if len(splitKeys) != 0 { - require.True(t, lastOverlappingWithQuerySp.span.Key.Compare(lastSplitKey.AsRawKey()) >= 0, - "expected to find %s sorted after %s", - lastOverlappingWithQuerySp.span.Key, lastSplitKey) + if len(splitKeys) != 0 { + require.True(t, lastOverlappingWithQuerySp.span.Key.Compare(lastSplitKey.AsRawKey()) >= 0, + "expected to find %s sorted after %s", + lastOverlappingWithQuerySp.span.Key, lastSplitKey) - // The config at the last split key must match the last overlapping config. - require.Truef(t, confAtLastSplitKey.Equal(lastOverlappingWithQuerySp.config), - "expected identical configs, found %s:%s and %s:%s", - lastSplitKey, spanconfigtestutils.PrintSpanConfig(confAtLastSplitKey), - lastOverlappingWithQuerySp.span.Key, spanconfigtestutils.PrintSpanConfig(lastOverlappingWithQuerySp.config), - ) - } + // The config at the last split key must match the last overlapping + // config, property (3.i). + require.Truef(t, confAtLastSplitKey.Equal(lastOverlappingWithQuerySp.config), + "expected identical configs, found %s:%s and %s:%s", + lastSplitKey, spanconfigtestutils.PrintSpanConfig(confAtLastSplitKey), + lastOverlappingWithQuerySp.span.Key, spanconfigtestutils.PrintSpanConfig(lastOverlappingWithQuerySp.config), + ) + } + }) } diff --git a/pkg/spanconfig/spanconfigstore/store.go b/pkg/spanconfig/spanconfigstore/store.go index 28034c5fd30c..02552a9b1450 100644 --- a/pkg/spanconfig/spanconfigstore/store.go +++ b/pkg/spanconfig/spanconfigstore/store.go @@ -37,7 +37,7 @@ var EnabledSetting = settings.RegisterBoolSetting( // Store is an in-memory data structure to store, retrieve, and incrementally // update the span configuration state. Internally, it makes use of an interval -// tree based spanConfigStore to store non-overlapping span configurations that +// btree based spanConfigStore to store non-overlapping span configurations that // target keyspans. It's safe for concurrent use. type Store struct { mu struct { @@ -88,7 +88,11 @@ func (s *Store) ComputeSplitKey(ctx context.Context, start, end roachpb.RKey) ro s.mu.RLock() defer s.mu.RUnlock() - return s.mu.spanConfigStore.computeSplitKey(ctx, start, end) + splitKey, err := s.mu.spanConfigStore.computeSplitKey(start, end) + if err != nil { + log.FatalfDepth(ctx, 3, "unable to compute split key: %v", err) + } + return splitKey } // GetSpanConfigForKey is part of the spanconfig.StoreReader interface. @@ -139,14 +143,14 @@ func (s *Store) ForEachOverlappingSpanConfig( }) } -// Copy returns a copy of the Store. -func (s *Store) Copy(ctx context.Context) *Store { +// Clone returns a copy of the Store. +func (s *Store) Clone() *Store { s.mu.Lock() defer s.mu.Unlock() clone := New(s.fallback, s.settings, s.knobs) - clone.mu.spanConfigStore = s.mu.spanConfigStore.copy(ctx) - clone.mu.systemSpanConfigStore = s.mu.systemSpanConfigStore.copy() + clone.mu.spanConfigStore = s.mu.spanConfigStore.clone() + clone.mu.systemSpanConfigStore = s.mu.systemSpanConfigStore.clone() return clone } @@ -183,7 +187,7 @@ func (s *Store) applyInternal( for _, entry := range addedEntries { record, err := spanconfig.MakeRecord( spanconfig.MakeTargetFromSpan(entry.span), - entry.conf(s.mu.spanConfigStore.interner), + entry.conf(), ) if err != nil { return nil, nil, err diff --git a/pkg/spanconfig/spanconfigstore/store_test.go b/pkg/spanconfig/spanconfigstore/store_test.go index c06fdfd1b6ba..1949eb71cb8f 100644 --- a/pkg/spanconfig/spanconfigstore/store_test.go +++ b/pkg/spanconfig/spanconfigstore/store_test.go @@ -36,22 +36,21 @@ func (s *Store) TestingApplyInternal( // TestingSplitKeys returns the computed list of range split points between // [start, end). -func (s *Store) TestingSplitKeys(ctx context.Context, start, end roachpb.RKey) []roachpb.RKey { +func (s *Store) TestingSplitKeys(tb testing.TB, start, end roachpb.RKey) []roachpb.RKey { s.mu.RLock() defer s.mu.RUnlock() - return s.mu.spanConfigStore.TestingSplitKeys(ctx, start, end) + return s.mu.spanConfigStore.TestingSplitKeys(tb, start, end) } // TestingSplitKeys returns the computed list of range split points between // [start, end). -func (s *spanConfigStore) TestingSplitKeys( - ctx context.Context, start, end roachpb.RKey, -) []roachpb.RKey { +func (s *spanConfigStore) TestingSplitKeys(tb testing.TB, start, end roachpb.RKey) []roachpb.RKey { var splitKeys []roachpb.RKey computeStart := start for { - splitKey := s.computeSplitKey(ctx, computeStart, end) + splitKey, err := s.computeSplitKey(computeStart, end) + require.NoError(tb, err) if splitKey == nil { break } @@ -100,6 +99,10 @@ func (s *spanConfigStore) TestingSplitKeys( // [d,f):B // [f,h):A // +// interned +// ---- +// A (refs = 2) +// B (refs = 1) // // Text of the form [a,b), {entire-keyspace}, {source=1,target=20}, and [a,b):C // correspond to targets {spans, system targets} and span config records; see @@ -170,7 +173,7 @@ func TestDataDriven(t *testing.T) { span := spanconfigtestutils.ParseSpan(t, spanStr) start, end := roachpb.RKey(span.Key), roachpb.RKey(span.EndKey) - splitKeys := store.TestingSplitKeys(ctx, start, end) + splitKeys := store.TestingSplitKeys(t, start, end) var b strings.Builder for _, splitKey := range splitKeys { b.WriteString(fmt.Sprintf("key=%s\n", string(splitKey))) @@ -194,6 +197,14 @@ func TestDataDriven(t *testing.T) { ) return strings.Join(results, "\n") + case "interned": + var b strings.Builder + for _, i := range store.testingInterned() { + b.WriteString(fmt.Sprintf("%s (refs = %d)\n", + spanconfigtestutils.PrintSpanConfig(i.SpanConfig), i.RefCount)) + } + return b.String() + default: t.Fatalf("unknown command: %s", d.Cmd) } @@ -248,7 +259,7 @@ func TestStoreClone(t *testing.T) { original := New(roachpb.TestingDefaultSpanConfig(), cluster.MakeClusterSettings(), nil) original.Apply(ctx, false, updates...) - clone := original.Copy(ctx) + clone := original.Clone() var originalRecords, clonedRecords []spanconfig.Record _ = original.Iterate(func(rec spanconfig.Record) error { @@ -272,6 +283,9 @@ func TestStoreClone(t *testing.T) { } } +// BenchmarkStoreComputeSplitKey measures how long it takes to compute the split +// key while varying the total number of span config entries we have to sift +// through for each computation. func BenchmarkStoreComputeSplitKey(b *testing.B) { ctx := context.Background() for _, numEntries := range []int{10_000, 100_000, 1_000_000} { @@ -311,3 +325,29 @@ func BenchmarkStoreComputeSplitKey(b *testing.B) { }) } } + +type interned struct { + roachpb.SpanConfig + RefCount uint64 +} + +func (s *Store) testingInterned() []interned { + s.mu.RLock() + defer s.mu.RUnlock() + + return s.mu.spanConfigStore.testingInterned() +} + +func (s *spanConfigStore) testingInterned() []interned { + var is []interned + for canonical, refs := range s.interner.refCounts { + is = append(is, interned{ + SpanConfig: *canonical, + RefCount: refs, + }) + } + sort.Slice(is, func(i, j int) bool { + return spanconfigtestutils.PrintSpanConfig(is[i].SpanConfig) < spanconfigtestutils.PrintSpanConfig(is[j].SpanConfig) + }) + return is +} diff --git a/pkg/spanconfig/spanconfigstore/system_store.go b/pkg/spanconfig/spanconfigstore/system_store.go index 42bb67df0102..9b8784a352c3 100644 --- a/pkg/spanconfig/spanconfigstore/system_store.go +++ b/pkg/spanconfig/spanconfigstore/system_store.go @@ -120,8 +120,8 @@ func (s *systemSpanConfigStore) combine( return config, nil } -// copy returns a copy of the system span config store. -func (s *systemSpanConfigStore) copy() *systemSpanConfigStore { +// clone returns a copy of the system span config store. +func (s *systemSpanConfigStore) clone() *systemSpanConfigStore { clone := newSystemSpanConfigStore() for k := range s.store { clone.store[k] = s.store[k] diff --git a/pkg/spanconfig/spanconfigstore/testdata/batched/interned b/pkg/spanconfig/spanconfigstore/testdata/batched/interned new file mode 100644 index 000000000000..11cfc1b26a20 --- /dev/null +++ b/pkg/spanconfig/spanconfigstore/testdata/batched/interned @@ -0,0 +1,81 @@ +# Tests to make sure config interning works as expected. We should be observe +# the right ref counts for each unique config. + +apply +set [a,b):A +set [b,c):B +set [c,d):C +set [d,e):D +set [e,f):D +set [f,g):D +set [g,h):D +---- +added [a,b):A +added [b,c):B +added [c,d):C +added [d,e):D +added [e,f):D +added [f,g):D +added [g,h):D + +interned +---- +A (refs = 1) +B (refs = 1) +C (refs = 1) +D (refs = 4) + +apply +delete [e,f) +delete [b,c) +---- +deleted [b,c) +deleted [e,f) + +interned +---- +A (refs = 1) +C (refs = 1) +D (refs = 3) + +apply +set [a,b):A +set [b,c):B +---- +added [b,c):B + +overlapping span=[a,z) +---- +[a,b):A +[b,c):B +[c,d):C +[d,e):D +[f,g):D +[g,h):D + +interned +---- +A (refs = 1) +B (refs = 1) +C (refs = 1) +D (refs = 3) + +apply +set [a,d):A +---- +deleted [a,b) +deleted [b,c) +deleted [c,d) +added [a,d):A + +overlapping span=[a,z) +---- +[a,d):A +[d,e):D +[f,g):D +[g,h):D + +interned +---- +A (refs = 1) +D (refs = 3) diff --git a/pkg/spanconfig/spanconfigstore/testdata/single/overlap b/pkg/spanconfig/spanconfigstore/testdata/single/overlap index 0b6700352ac2..7ed98dfdde3b 100644 --- a/pkg/spanconfig/spanconfigstore/testdata/single/overlap +++ b/pkg/spanconfig/spanconfigstore/testdata/single/overlap @@ -23,6 +23,11 @@ overlapping span=[b,h) [d,f):B [f,h):A +interned +---- +A (refs = 2) +B (refs = 1) + # Check that writing a span that partially overlaps with multiple existing # entries deletes all of them, and re-adds the right non-overlapping fragments @@ -43,6 +48,12 @@ overlapping span=[b,h) [e,f):B [f,h):A +interned +---- +A (refs = 2) +B (refs = 1) +C (refs = 1) + # Check that when a span being written to entirely envelopes an existing entry, # that entry is deleted in its entirety. apply @@ -60,6 +71,11 @@ overlapping span=[b,h) [c,d):C [g,h):A +interned +---- +A (refs = 2) +C (refs = 1) + # Validate that the right split points (span start keys) are surfaced. needs-split span=[b,h) ---- @@ -81,6 +97,11 @@ overlapping span=[b,h) [b,g):B [g,h):A +interned +---- +A (refs = 1) +B (refs = 1) + needs-split span=[b,h) ---- true diff --git a/pkg/spanconfig/spanconfigstore/testdata/single/split_keys b/pkg/spanconfig/spanconfigstore/testdata/single/split_keys index 94274516e160..fdd4d7b0fec9 100644 --- a/pkg/spanconfig/spanconfigstore/testdata/single/split_keys +++ b/pkg/spanconfig/spanconfigstore/testdata/single/split_keys @@ -59,6 +59,14 @@ overlapping span=[a,z) [f,g):E [g,h):D +interned +---- +A (refs = 1) +B (refs = 1) +C (refs = 1) +D (refs = 2) +E (refs = 2) + split-keys span=[a,z) ---- key=b