Merge #157077 #157086

157077: sql: remove gossip-based DistSQL physical planning r=yuzefovich a=yuzefovich

In 25.1 release we switched to using SQL-instance-based DistSQL physical planning but left gossip-based planning just in case (controlled via a cluster setting). We haven't seen any fallout from that, and given that many different workloads have been upgraded to 25.2, it seems safe to completely remove gossip-based planning now.

Epic: None
Release note: None

157086: go.mod: bump Pebble to f11535e4f8ec r=annrpom a=annrpom

Changes:

 * [`f11535e4`](cockroachdb/pebble@f11535e4) db: use Peek in TestVersionSetSeqNums
 * [`1ba71aa2`](cockroachdb/pebble@1ba71aa2) wal: consolidate LogRecycler.[Set]MinRecycleLogNum
 * [`82d592ec`](cockroachdb/pebble@82d592ec) db: document that multiple flushableEntries may share a logNum
 * [`9b012186`](cockroachdb/pebble@9b012186) db: add metric encapsulating blob file rewrite bytes read, written
 * [`b0c3d6c1`](cockroachdb/pebble@b0c3d6c1) db: disable writing to value blocks if `DisableSeparationBySuffix` is true
 * [`562c8d4d`](cockroachdb/pebble@562c8d4d) internal/manifest: document relationship between LastSeqNum and WAL seqnums
 * [`6d692923`](cockroachdb/pebble@6d692923) github: make sure the AI review bot picks up a PR with label added after creation

Release note: none.
Epic: none.

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Annie Pompa <[email protected]>

Author: 3 people

DEPS.bzl

@@ -1834,10 +1834,10 @@ def go_deps():
         patches = [
             "@com_github_cockroachdb_cockroach//build/patches:com_github_cockroachdb_pebble.patch",
         ],
-        sha256 = "e0ee903c366abc160446c82a18d98cb1fc4d3c44bf21d2748844354fcc4c9424",
-        strip_prefix = "github.com/cockroachdb/[email protected]20251105164901-5c1580d5c101",
+        sha256 = "fcc90273d0bed2095e4df64560b8c1bda9c8617cbd85837536fda2873ba9b3ee",
+        strip_prefix = "github.com/cockroachdb/[email protected]20251107191559-f11535e4f8ec",
         urls = [
-            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251105164901-5c1580d5c101.zip",
+            "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251107191559-f11535e4f8ec.zip",
         ],
     )
     go_repository(

build/bazelutil/distdir_files.bzl

@@ -352,7 +352,7 @@ DISTDIR_FILES = {
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/gostdlib/com_github_cockroachdb_gostdlib-v1.19.0.zip": "c4d516bcfe8c07b6fc09b8a9a07a95065b36c2855627cb3514e40c98f872b69e",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/logtags/com_github_cockroachdb_logtags-v0.0.0-20241215232642-bb51bb14a506.zip": "920068af09e3846d9ebb4e4a7787ff1dd10f3989c5f940ad861b0f6a9f824f6e",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/metamorphic/com_github_cockroachdb_metamorphic-v0.0.0-20231108215700-4ba948b56895.zip": "28c8cf42192951b69378cf537be5a9a43f2aeb35542908cc4fe5f689505853ea",
-    "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251105164901-5c1580d5c101.zip": "e0ee903c366abc160446c82a18d98cb1fc4d3c44bf21d2748844354fcc4c9424",
+    "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/pebble/com_github_cockroachdb_pebble-v0.0.0-20251107191559-f11535e4f8ec.zip": "fcc90273d0bed2095e4df64560b8c1bda9c8617cbd85837536fda2873ba9b3ee",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/redact/com_github_cockroachdb_redact-v1.1.6.zip": "018eccb5fb9ca52d43ec9eaf213539d01c1f2b94e0e822406ebfb2e9321ef6cf",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/returncheck/com_github_cockroachdb_returncheck-v0.0.0-20200612231554-92cdbca611dd.zip": "ce92ba4352deec995b1f2eecf16eba7f5d51f5aa245a1c362dfe24c83d31f82b",
     "https://storage.googleapis.com/cockroach-godeps/gomod/github.com/cockroachdb/stress/com_github_cockroachdb_stress-v0.0.0-20220803192808-1806698b1b7b.zip": "3fda531795c600daf25532a4f98be2a1335cd1e5e182c72789bca79f5f69fcc1",

go.mod

@@ -137,7 +137,7 @@ require (
 	github.com/cockroachdb/errors v1.12.1-0.20251010171200-64801262cd6f
 	github.com/cockroachdb/gostdlib v1.19.0
 	github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506
-	github.com/cockroachdb/pebble v0.0.0-20251105164901-5c1580d5c101
+	github.com/cockroachdb/pebble v0.0.0-20251107191559-f11535e4f8ec
 	github.com/cockroachdb/redact v1.1.6
 	github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd
 	github.com/cockroachdb/tokenbucket v0.0.0-20250429170803-42689b6311bb

go.sum

@@ -572,8 +572,8 @@ github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506 h1:ASDL+UJcILM
 github.com/cockroachdb/logtags v0.0.0-20241215232642-bb51bb14a506/go.mod h1:Mw7HqKr2kdtu6aYGn3tPmAftiP3QPX63LdK/zcariIo=
 github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895 h1:XANOgPYtvELQ/h4IrmPAohXqe2pWA8Bwhejr3VQoZsA=
 github.com/cockroachdb/metamorphic v0.0.0-20231108215700-4ba948b56895/go.mod h1:aPd7gM9ov9M8v32Yy5NJrDyOcD8z642dqs+F0CeNXfA=
-github.com/cockroachdb/pebble v0.0.0-20251105164901-5c1580d5c101 h1:k/gEqWA60iDwUdT3OgEIWirEUkI4ku/fxFfut7it6sg=
-github.com/cockroachdb/pebble v0.0.0-20251105164901-5c1580d5c101/go.mod h1:VCfKgAYZuLVBLitd8boLrNa0oE3OsnRaQauWXvFQWKc=
+github.com/cockroachdb/pebble v0.0.0-20251107191559-f11535e4f8ec h1:9n1chvEvKnZf+qO9tcnKNoK00muKOqCZ+lQRtLaQypM=
+github.com/cockroachdb/pebble v0.0.0-20251107191559-f11535e4f8ec/go.mod h1:VCfKgAYZuLVBLitd8boLrNa0oE3OsnRaQauWXvFQWKc=
 github.com/cockroachdb/redact v1.1.6 h1:zXJBwDZ84xJNlHl1rMyCojqyIxv+7YUpQiJLQ7n4314=
 github.com/cockroachdb/redact v1.1.6/go.mod h1:BVNblN9mBWFyMyqK1k3AAiSxhvhfK2oOZZ2lK+dpvRg=
 github.com/cockroachdb/returncheck v0.0.0-20200612231554-92cdbca611dd h1:KFOt5I9nEKZgCnOSmy8r4Oykh8BYQO8bFOTgHDS8YZA=

pkg/settings/registry.go

@@ -274,6 +274,9 @@ var retiredSettings = map[InternalKey]struct{}{
 
 	// removed as of 25.4
 	"storage.columnar_blocks.enabled": {},
+
+	// removed as of 26.1
+	"sql.distsql_planning.use_gossip.enabled": {},
 }
 
 // grandfatheredDefaultSettings is the list of "grandfathered" existing sql.defaults

pkg/sql/distsql_physical_planner.go

@@ -1314,13 +1314,6 @@ const (
 	// SpanPartitionReason_ROUND_ROBIN is reported when there is no locality info
 	// on any of the instances and so we default to a naive round-robin strategy.
 	SpanPartitionReason_ROUND_ROBIN
-	// SpanPartitionReason_GOSSIP_GATEWAY_TARGET_UNHEALTHY is reported when the
-	// target node retrieved via gossip is deemed unhealthy. In this case we
-	// default to the gateway node.
-	SpanPartitionReason_GOSSIP_GATEWAY_TARGET_UNHEALTHY
-	// SpanPartitionReason_GOSSIP_TARGET_HEALTHY is reported when the
-	// target node retrieved via gossip is deemed healthy.
-	SpanPartitionReason_GOSSIP_TARGET_HEALTHY
 	// SpanPartitionReason_LOCALITY_FILTERED_RANDOM_GATEWAY_OVERLOADED is reported
 	// when there is no match to the provided locality filter and the gateway is
 	// eligible but overloaded with other partitions. In this case we pick a
@@ -1350,10 +1343,6 @@ func (r SpanPartitionReason) String() string {
 		return "locality-filtered-random"
 	case SpanPartitionReason_ROUND_ROBIN:
 		return "round-robin"
-	case SpanPartitionReason_GOSSIP_GATEWAY_TARGET_UNHEALTHY:
-		return "gossip-gateway-target-unhealthy"
-	case SpanPartitionReason_GOSSIP_TARGET_HEALTHY:
-		return "gossip-target-healthy"
 	case SpanPartitionReason_LOCALITY_FILTERED_RANDOM_GATEWAY_OVERLOADED:
 		return "locality-filtered-random-gateway-overloaded"
 	default:
@@ -1527,9 +1516,6 @@ func (dsp *DistSQLPlanner) partitionSpansEx(
 		return []SpanPartition{{SQLInstanceID: dsp.gatewaySQLInstanceID, Spans: spans}},
 			true /* ignoreMisplannedRanges */, nil
 	}
-	if dsp.useGossipPlanning(ctx, planCtx) {
-		return dsp.deprecatedPartitionSpansSystem(ctx, planCtx, spans)
-	}
 	return dsp.partitionSpans(ctx, planCtx, spans, bound)
 }
 
@@ -1651,27 +1637,6 @@ func (dsp *DistSQLPlanner) partitionSpan(
 	return partitions, lastPartitionIdx, nil
 }
 
-// deprecatedPartitionSpansSystem finds node owners for ranges touching the given spans
-// for a system tenant.
-func (dsp *DistSQLPlanner) deprecatedPartitionSpansSystem(
-	ctx context.Context, planCtx *PlanningCtx, spans roachpb.Spans,
-) (partitions []SpanPartition, ignoreMisplannedRanges bool, _ error) {
-	nodeMap := make(map[base.SQLInstanceID]int)
-	resolver := func(nodeID roachpb.NodeID) (base.SQLInstanceID, SpanPartitionReason) {
-		return dsp.deprecatedHealthySQLInstanceIDForKVNodeIDSystem(ctx, planCtx, nodeID)
-	}
-	for _, span := range spans {
-		var err error
-		partitions, _, err = dsp.partitionSpan(
-			ctx, planCtx, span, partitions, nodeMap, resolver, &ignoreMisplannedRanges,
-		)
-		if err != nil {
-			return nil, false, err
-		}
-	}
-	return partitions, ignoreMisplannedRanges, nil
-}
-
 // partitionSpans assigns SQL instances to spans. In mixed sql and KV mode it
 // generally assigns each span to the instance hosted on the KV node chosen by
 // the configured replica oracle, while in clusters operating with standalone
@@ -1730,26 +1695,6 @@ func (dsp *DistSQLPlanner) partitionSpans(
 	return partitions, ignoreMisplannedRanges, nil
 }
 
-// deprecatedHealthySQLInstanceIDForKVNodeIDSystem returns the SQL instance that
-// should handle the range with the given node ID when planning is done on
-// behalf of the system tenant. It ensures that the chosen SQL instance is
-// healthy and of the compatible DistSQL version.
-func (dsp *DistSQLPlanner) deprecatedHealthySQLInstanceIDForKVNodeIDSystem(
-	ctx context.Context, planCtx *PlanningCtx, nodeID roachpb.NodeID,
-) (base.SQLInstanceID, SpanPartitionReason) {
-	sqlInstanceID := base.SQLInstanceID(nodeID)
-	status := dsp.checkInstanceHealthAndVersionSystem(ctx, planCtx, sqlInstanceID)
-	// If the node is unhealthy, use the gateway to process this span instead of
-	// the unhealthy host. An empty address indicates an unhealthy host.
-	reason := SpanPartitionReason_GOSSIP_TARGET_HEALTHY
-	if status != NodeOK {
-		log.VEventf(ctx, 2, "not planning on node %d: %s", sqlInstanceID, status)
-		sqlInstanceID = dsp.gatewaySQLInstanceID
-		reason = SpanPartitionReason_GOSSIP_GATEWAY_TARGET_UNHEALTHY
-	}
-	return sqlInstanceID, reason
-}
-
 // checkInstanceHealth returns the instance health status by dialing the node.
 // It also caches the result to avoid redialing for a query.
 func (dsp *DistSQLPlanner) checkInstanceHealth(
@@ -2129,10 +2074,6 @@ func (dsp *DistSQLPlanner) getInstanceIDForScan(
 		return 0, err
 	}
 
-	if dsp.useGossipPlanning(ctx, planCtx) {
-		sqlInstanceID, _ := dsp.deprecatedHealthySQLInstanceIDForKVNodeIDSystem(ctx, planCtx, replDesc.NodeID)
-		return sqlInstanceID, nil
-	}
 	resolver, err := dsp.makeInstanceResolver(ctx, planCtx)
 	if err != nil {
 		return 0, err
@@ -2142,23 +2083,6 @@ func (dsp *DistSQLPlanner) getInstanceIDForScan(
 	return sqlInstanceID, nil
 }
 
-// TODO(yuzefovich): retire this setting altogether in 25.3 release.
-var useGossipPlanning = settings.RegisterBoolSetting(
-	settings.ApplicationLevel,
-	"sql.distsql_planning.use_gossip.enabled",
-	"if enabled, the DistSQL physical planner falls back to gossip-based planning",
-	false,
-)
-
-func (dsp *DistSQLPlanner) useGossipPlanning(_ context.Context, planCtx *PlanningCtx) bool {
-	var gossipPlanningEnabled bool
-	// Some of the planCtx fields can be left unset in tests.
-	if planCtx.ExtendedEvalCtx != nil && planCtx.ExtendedEvalCtx.Settings != nil {
-		gossipPlanningEnabled = useGossipPlanning.Get(&planCtx.ExtendedEvalCtx.Settings.SV)
-	}
-	return dsp.codec.ForSystemTenant() && planCtx.localityFilter.Empty() && gossipPlanningEnabled
-}
-
 // convertOrdering maps the columns in props.ordering to the output columns of a
 // processor.
 func (dsp *DistSQLPlanner) convertOrdering(

pkg/sql/distsql_physical_planner_test.go

@@ -19,7 +19,6 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
-	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/rangecache"
@@ -1751,112 +1750,6 @@ func TestShouldPickGatewayNode(t *testing.T) {
 	}
 }
 
-// Test that a node whose descriptor info is not accessible through gossip is
-// not used. This is to simulate nodes that have been decomisioned and also
-// nodes that have been "replaced" by another node at the same address (which, I
-// guess, is also a type of decomissioning).
-func TestPartitionSpansSkipsNodesNotInGossip(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	// The spans that we're going to plan for.
-	span := roachpb.Span{Key: roachpb.Key("A"), EndKey: roachpb.Key("Z")}
-	gatewayNode := roachpb.NodeID(2)
-	ranges := []testSpanResolverRange{{"A", 1}, {"B", 2}, {"C", 1}}
-
-	stopper := stop.NewStopper()
-	defer stopper.Stop(context.Background())
-	codec := keys.SystemSQLCodec
-
-	mockGossip := gossip.NewTest(roachpb.NodeID(1), stopper, metric.NewRegistry())
-	var nodeDescs []*roachpb.NodeDescriptor
-	for i := 1; i <= 2; i++ {
-		sqlInstanceID := base.SQLInstanceID(i)
-		desc := &roachpb.NodeDescriptor{
-			NodeID:  roachpb.NodeID(sqlInstanceID),
-			Address: util.UnresolvedAddr{AddressField: fmt.Sprintf("addr%d", i)},
-		}
-		if i == 2 {
-			if err := mockGossip.SetNodeDescriptor(desc); err != nil {
-				t.Fatal(err)
-			}
-		}
-		// All the nodes advertise that they are not draining. This is to
-		// simulate the "node overridden by another node at the same address"
-		// case mentioned in the test comment - for such a node, the descriptor
-		// would be taken out of the gossip data, but other datums it advertised
-		// are left in place.
-		if err := mockGossip.AddInfoProto(
-			gossip.MakeDistSQLDrainingKey(sqlInstanceID),
-			&execinfrapb.DistSQLDrainingInfo{
-				Draining: false,
-			},
-			0, // ttl - no expiration
-		); err != nil {
-			t.Fatal(err)
-		}
-
-		nodeDescs = append(nodeDescs, desc)
-	}
-	tsp := &testSpanResolver{
-		nodes:  nodeDescs,
-		ranges: ranges,
-	}
-
-	st := cluster.MakeTestingClusterSettings()
-	gw := gossip.MakeOptionalGossip(mockGossip)
-	dsp := DistSQLPlanner{
-		st:                   st,
-		gatewaySQLInstanceID: base.SQLInstanceID(tsp.nodes[gatewayNode-1].NodeID),
-		stopper:              stopper,
-		spanResolver:         tsp,
-		gossip:               gw,
-		nodeHealth: distSQLNodeHealth{
-			gossip: gw,
-			connHealthSystem: func(node roachpb.NodeID, _ rpcbase.ConnectionClass) error {
-				_, _, err := mockGossip.GetNodeIDAddress(node)
-				return err
-			},
-			isAvailable: func(base.SQLInstanceID) bool {
-				return true
-			},
-		},
-		codec: codec,
-	}
-
-	ctx := context.Background()
-	// This test is specific to gossip-based planning.
-	useGossipPlanning.Override(ctx, &st.SV, true)
-	planCtx := dsp.NewPlanningCtx(
-		ctx, &extendedEvalContext{Context: eval.Context{Codec: codec, Settings: st}},
-		nil /* planner */, nil /* txn */, FullDistribution,
-	)
-	partitions, err := dsp.PartitionSpans(ctx, planCtx, roachpb.Spans{span}, PartitionSpansBoundDefault)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	resMap := make(map[base.SQLInstanceID][][2]string)
-	for _, p := range partitions {
-		if _, ok := resMap[p.SQLInstanceID]; ok {
-			t.Fatalf("node %d shows up in multiple partitions", p.SQLInstanceID)
-		}
-		var spans [][2]string
-		for _, s := range p.Spans {
-			spans = append(spans, [2]string{string(s.Key), string(s.EndKey)})
-		}
-		resMap[p.SQLInstanceID] = spans
-	}
-
-	expectedPartitions :=
-		map[base.SQLInstanceID][][2]string{
-			2: {{"A", "Z"}},
-		}
-	if !reflect.DeepEqual(resMap, expectedPartitions) {
-		t.Errorf("expected partitions:\n  %v\ngot:\n  %v", expectedPartitions, resMap)
-	}
-}
-
 func TestCheckNodeHealth(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)

pkg/sql/distsql_plan_bulk.go

@@ -41,6 +41,8 @@ func (dsp *DistSQLPlanner) SetupAllNodesPlanningWithOracle(
 	localityFilter roachpb.Locality,
 ) (*PlanningCtx, []base.SQLInstanceID, error) {
 	if dsp.codec.ForSystemTenant() {
+		// TODO(yuzefovich): evaluate whether we can remove system tenant
+		// specific code.
 		return dsp.setupAllNodesPlanningSystem(ctx, evalCtx, execCfg, oracle, localityFilter)
 	}
 	return dsp.setupAllNodesPlanningTenant(ctx, evalCtx, execCfg, oracle, localityFilter)