spanconfigkvsubscriber,kvserver: fix KVSubscriber bug

We had a bug in the KVSubscriber where we were invoking a copy of the
handler instead of the handler stored. This meant that we'd never treat
handlers as "initialized". As a result, we would always invoke them with
the everything span, and as a result, visit all replicas on the stores
in reaction to span config updates. See datadriven test diffs for an
illustration.

Fixing the above lead to unearthing an interesting bug in how we were
deciding to enqueue replicas in the split queue. Previously, if we
received  a span config update that implied a split and the update
corresponded to the right-hand side post split, we would skip enqueuing
the replica in the split queue. The assumption was that we'd get an
update corresponding to the LHS of the split for the same replica
and that update would enqueue the replica. This doesn't always hold true
though. For example, consider the case when a new table is created and
must be split from its (left) adjacent table's range. This only results
in a single update, corresponding to the new table's span, which is the
right-hand side post split.

This patch moves to nudging the split queue for all updates, not just
left-hand side updates, for the reason above.

Release note: None

Release justification: bug fixes in new functionality

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

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())

pkg/spanconfig/spanconfigkvsubscriber/testdata/basic

@@ -40,7 +40,6 @@ delete [d,f)
 
 updates
 ----
-[/Min,/Max)
 [d,f)
 
 store-reader key=d

pkg/spanconfig/spanconfigkvsubscriber/testdata/buffer_overflow

@@ -75,7 +75,6 @@ upsert [a,c):C
 
 updates
 ----
-[/Min,/Max)
 [a,c)
 
 store-reader key=a

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

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)