diff --git a/pkg/kv/kvserver/store.go b/pkg/kv/kvserver/store.go index cb2c980c64e7..a368853dd4e1 100644 --- a/pkg/kv/kvserver/store.go +++ b/pkg/kv/kvserver/store.go @@ -2395,45 +2395,43 @@ func (s *Store) onSpanConfigUpdate(ctx context.Context, updated roachpb.Span) { return nil // placeholder; ignore } + replCtx := repl.AnnotateCtx(ctx) startKey := repl.Desc().StartKey - if !sp.ContainsKey(startKey) { - // It's possible that the update we're receiving here is the - // right-hand side of a span config getting split. Think of - // installing a zone config on some partition of an index where - // previously there was none on any of the partitions. The range - // spanning the entire index would have to split on the - // partition boundary, and before it does so, it's possible that - // it would receive a span config update for just the partition. + if sp.ContainsKey(startKey) { + // It's possible that the update we're receiving here implies a split. + // If the update corresponds to what would be the config for the + // right-hand side after the split, we avoid clobbering the pre-split + // range's embedded span config by checking if the start key is part of + // the update. // - // To avoid clobbering the pre-split range's embedded span - // config with the partition's config, we'll ensure that the - // range's start key is part of the update. We don't have to - // enqueue the range in the split queue here, that takes place - // when processing the left-hand side span config update. - - return nil // ignore - } - - // TODO(irfansharif): It's possible for a config to be applied over an - // entire range when it only pertains to the first half of the range. - // This will be corrected shortly -- we enqueue the range for a split - // below where we then apply the right config on each half. But still, - // it's surprising behavior and gets in the way of a desirable - // consistency guarantee: a key's config at any point in time is one - // that was explicitly declared over it, or the default config. - // - // We can do better, we can skip applying the config entirely and - // enqueue the split, then relying on the split trigger to install - // the right configs on each half. The current structure is as it is - // to maintain parity with the system config span variant. - - replCtx := repl.AnnotateCtx(ctx) - conf, err := s.cfg.SpanConfigSubscriber.GetSpanConfigForKey(replCtx, startKey) - if err != nil { - log.Errorf(ctx, "skipped applying update, unexpected error reading from subscriber: %v", err) - return err + // Even if we're dealing with what would be the right-hand side after + // the split is processed, we still want to nudge the split queue + // below -- we can't instead rely on there being an update for the + // left-hand side of the split. Concretely, consider the case when a + // new table is added with a different configuration to its (left) + // adjacent table. This results in a single update, corresponding to the + // new table's span, which forms the right-hand side post split. + + // TODO(irfansharif): It's possible for a config to be applied over an + // entire range when it only pertains to the first half of the range. + // This will be corrected shortly -- we enqueue the range for a split + // below where we then apply the right config on each half. But still, + // it's surprising behavior and gets in the way of a desirable + // consistency guarantee: a key's config at any point in time is one + // that was explicitly declared over it, or the default config. + // + // We can do better, we can skip applying the config entirely and + // enqueue the split, then relying on the split trigger to install + // the right configs on each half. The current structure is as it is + // to maintain parity with the system config span variant. + + conf, err := s.cfg.SpanConfigSubscriber.GetSpanConfigForKey(replCtx, startKey) + if err != nil { + log.Errorf(ctx, "skipped applying update, unexpected error reading from subscriber: %v", err) + return err + } + repl.SetSpanConfig(conf) } - repl.SetSpanConfig(conf) // TODO(irfansharif): For symmetry with the system config span variant, // we queue blindly; we could instead only queue it if we knew the diff --git a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go index 84ca69e3eaf7..0714c2bdbcec 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go +++ b/pkg/spanconfig/spanconfigkvsubscriber/kvsubscriber.go @@ -229,8 +229,9 @@ func (s *KVSubscriber) handleCompleteUpdate( s.mu.lastUpdated = ts handlers := s.mu.handlers s.mu.Unlock() - for _, h := range handlers { - h.invoke(ctx, keys.EverythingSpan) + for i := range handlers { + handler := &handlers[i] // mutated by invoke + handler.invoke(ctx, keys.EverythingSpan) } } @@ -248,7 +249,8 @@ func (s *KVSubscriber) handlePartialUpdate( handlers := s.mu.handlers s.mu.Unlock() - for _, handler := range handlers { + for i := range handlers { + handler := &handlers[i] // mutated by invoke for _, ev := range events { target := ev.(*bufferEvent).Update.Target handler.invoke(ctx, target.KeyspaceTargeted()) diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic b/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic index d188e8dcaeed..54d052c09945 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/basic @@ -40,7 +40,6 @@ delete [d,f) updates ---- -[/Min,/Max) [d,f) store-reader key=d diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow b/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow index 7a1b0d5397c4..997caa397365 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow @@ -75,7 +75,6 @@ upsert [a,c):C updates ---- -[/Min,/Max) [a,c) store-reader key=a diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs index d345d8533e8d..200a7c790b76 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs @@ -55,7 +55,6 @@ upsert {source=1, target=1}:Y # [/Min,/Tenant/2] corresponds to the system tenant targeting itself. updates ---- -[/Min,/Max) [/Min,/Tenant/2) # Ensure configs are correctly hydrated when we read them. @@ -93,7 +92,6 @@ upsert {source=1, target=1}:V updates ---- -[/Min,/Max) [/Min,/Tenant/2) store-reader key=a diff --git a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants index 14fe1bfd8433..708e288b768d 100644 --- a/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants +++ b/pkg/spanconfig/spanconfigkvsubscriber/testdata/system_span_configs_secondary_tenants @@ -21,7 +21,6 @@ delete {source=1,target=20} updates ---- -[/Min,/Max) [/Tenant/20,/Tenant/21) # Lastly, update a system span config set on a secondary tenant's keyspace and @@ -32,5 +31,4 @@ upsert {source=10, target=10}:U updates ---- -[/Min,/Max) [/Tenant/10,/Tenant/11)