Merge #153373

153373:  kvserver: resolve range-size backpressure preventing spanconfig updates r=iskettaneh a=dodeca12

This PR ensures that all spanconfig updates bypass the backpressure mechanism. This is done by excluding the `system.span_configurations` table from backpressurable spans by splitting the backpressurable range around it.

#### Implementation

##### Commit 1: Reproduce the Issue

- Added comprehensive test TestSpanConfigUpdatesBlockedByRangeSizeBackpressureOnDefaultRanges that reproduces the catch-22 scenario
- The test repeatedly writes spanconfig updates for a single key until the range becomes too large to split, triggering backpressure
- Demonstrates how spanconfig updates (including protected timestamp deletions) get blocked by backpressure:
```log
W250910 14:26:03.302675 144058 kv/kvserver/replica_backpressure.go:263  [T1,Vsystem,n1,s1,r77/1:/Table/4{7/1/"\xf…-8}] 481  applying backpressure to limit range growth on batch Put [/Table/47/1/"\xfa"/0], [txn: cfbe5191], [can-forward-ts]
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482  ERROR! BREAKING OUT OF LOOP, numWrites successful: 62690, error: upsert-span-cfgs: split failed while applying backpressure to Put [/Table/47/1/"\xfa"/0], [txn: cfbe5191], [can-forward-ts] on range r77:/Table/4{7/1/"\xfa"-8} [(n1,s1):1, next=2, gen=1]: could not find valid split key
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +(1) attached stack trace
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  -- stack trace:
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | github.com/cockroachdb/cockroach/pkg/sql.(*InternalExecutor).execInternal.func1.1
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | 	pkg/sql/internal.go:1260
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | github.com/cockroachdb/cockroach/pkg/sql.(*rowsIterator).Close
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | 	pkg/sql/internal.go:608
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | github.com/cockroachdb/cockroach/pkg/sql.(*rowsIterator).Next
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | 	pkg/sql/internal.go:584
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | github.com/cockroachdb/cockroach/pkg/sql.(*InternalExecutor).execIEStmt
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | 	pkg/sql/internal.go:872
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | github.com/cockroachdb/cockroach/pkg/sql.(*InternalExecutor).ExecEx
I250910 14:26:03.304650 50 kv/kvserver_test/client_spanconfig_backpressure_test.go:209  [-] 482 +  | 	pkg/sql/internal.go:820
.
.
.
```

##### Commit 2: 

- **Modified `backpressurableSpans`** in `replica_backpressure.go`: Split the original backpressurable span `{SystemConfigTableDataMax, TableDataMax}` into two spans that exclude the span_configurations table:
  - `{SystemConfigTableDataMax, span_configurations_table_start}`
  - `{span_configurations_table_end, TableDataMax}`
- **Added explanatory comments**: Documented that this prevents catch-22 situations where protected timestamp updates or garbage collection TTL updates are blocked by backpressure
- **Updated test expectations**: Modified the aforementioned test to expect that spanconfig updates should succeed (bypass backpressure) rather than fail


Expected behaviour - any span config updates now bypass the range-size backpressure mechanism.

Fixes: #146982
Release note: None


Co-authored-by: Swapneeth Gorantla <[email protected]>

Author: craig[bot]

pkg/keys/constants.go

@@ -374,6 +374,10 @@ var (
 	NamespaceTableMin = SystemSQLCodec.TablePrefix(NamespaceTableID)
 	// NamespaceTableMax is the end key of system.namespace.
 	NamespaceTableMax = SystemSQLCodec.TablePrefix(NamespaceTableID + 1)
+	// SpanConfigTableMin is the start key of system.span_configurations.
+	SpanConfigTableMin = SystemSQLCodec.TablePrefix(SpanConfigurationsTableID)
+	// SpanConfigTableMax is the end key of system.span_configurations.
+	SpanConfigTableMax = SystemSQLCodec.TablePrefix(SpanConfigurationsTableID + 1)
 
 	// 4. Non-system tenant SQL keys
 	//

pkg/kv/kvserver/client_replica_backpressure_test.go

@@ -8,27 +8,36 @@ package kvserver_test
 import (
 	"context"
 	"fmt"
+	math "math"
 	"net/url"
+	"strings"
 	"sync"
 	"sync/atomic"
 	"testing"
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/keys"
+	"github.com/cockroachdb/cockroach/pkg/kv/kvclient/kvcoord"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/spanconfig"
+	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/pgurlutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
+	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/randutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/errors"
+	"github.com/dustin/go-humanize"
 	"github.com/jackc/pgx/v5"
 	"github.com/stretchr/testify/require"
 )
@@ -332,3 +341,233 @@ func TestBackpressureNotAppliedWhenReducingRangeSize(t *testing.T) {
 		require.Error(t, <-upsertErrCh)
 	})
 }
+
+// TestSpanConfigUpdatesDoNotGetBlockByRangeSizeBackpressureOnDefaultRanges
+// verifies that spanconfig updates do not block by backpressure when the
+// `system.span_configurations` table range becomes full, showing the allowlist
+// is working.
+//
+// Test strategy:
+//  1. Configure `system.span_configurations` table range to be a small size (8 KiB).
+//  2. Write many large spanconfig records (2 KiB each) to fill up the range.
+//  3. Verify spanconfig updates do not fail due to backpressure when the range is full,
+//  4. This test recreates the scenario where spanconfig updates do not fail due to
+//     backpressure.
+func TestSpanConfigUpdatesDoNotGetBlockByRangeSizeBackpressureOnDefaultRanges(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	const (
+		overloadMaxRangeBytes = 8 << 10   // 8 KiB, a saner value than default 512 MiB for testing
+		overloadMinRangeBytes = 2 << 10   // 2 KiB
+		numWrites             = 16        // enough to hit backpressure for 8 KiB range & 2 KiB spanconfig
+		defaultMaxBytes       = 512 << 20 // default max bytes for a range
+	)
+	s := serverutils.StartServerOnly(t, base.TestServerArgs{})
+	defer s.Stopper().Stop(ctx)
+
+	store, err := s.GetStores().(*kvserver.Stores).GetStore(s.GetFirstStoreID())
+	require.NoError(t, err)
+
+	waitForSpanConfig := func(t *testing.T, tc serverutils.TestServerInterface,
+		tablePrefix roachpb.Key, expRangeMaxBytes int64) {
+		testutils.SucceedsSoon(t, func() error {
+			_, r := getFirstStoreReplica(t, tc, tablePrefix)
+			conf, err := r.LoadSpanConfig(ctx)
+			if err != nil {
+				return err
+			}
+			if conf.RangeMaxBytes != expRangeMaxBytes {
+				return fmt.Errorf("expected RangeMaxBytes %d, got %d",
+					expRangeMaxBytes, conf.RangeMaxBytes)
+			}
+			return nil
+		})
+	}
+
+	spanConfigTablePrefix := keys.SystemSQLCodec.TablePrefix(
+		keys.SpanConfigurationsTableID)
+
+	t.Logf("targeting span_configurations table at key: %s (table ID %d)\n",
+		spanConfigTablePrefix, keys.SpanConfigurationsTableID)
+
+	scratchKey, err := s.ScratchRange()
+	require.NoError(t, err)
+
+	testutils.SucceedsSoon(t, func() error {
+		repl := store.LookupReplica(roachpb.RKey(scratchKey))
+		if got := repl.GetMaxBytes(ctx); got != defaultMaxBytes {
+			return errors.Errorf(
+				"range max bytes values did not start at %d; got %d",
+				defaultMaxBytes, got)
+		}
+		return nil
+	})
+
+	systemSpanConfigurationsTableSpan := roachpb.Span{
+		Key:    spanConfigTablePrefix,
+		EndKey: spanConfigTablePrefix.PrefixEnd(),
+	}
+
+	target := spanconfig.MakeTargetFromSpan(systemSpanConfigurationsTableSpan)
+
+	systemSpanConfig := roachpb.SpanConfig{
+		RangeMaxBytes: overloadMaxRangeBytes,
+		RangeMinBytes: overloadMinRangeBytes,
+	}
+
+	configBytessdfsdf, err := protoutil.Marshal(&systemSpanConfig)
+	require.NoError(t, err)
+	t.Logf("marshalled systemSpanConfig size: %d bytes", len(configBytessdfsdf))
+
+	record, err := spanconfig.MakeRecord(target, systemSpanConfig)
+	require.NoError(t, err)
+
+	kvaccessor := s.SpanConfigKVAccessor().(spanconfig.KVAccessor)
+
+	err = kvaccessor.UpdateSpanConfigRecords(
+		ctx, []spanconfig.Target{target},
+		[]spanconfig.Record{record}, hlc.MinTimestamp, hlc.MaxTimestamp)
+	require.NoError(t, err)
+
+	waitForSpanConfig(t, s, spanConfigTablePrefix, overloadMaxRangeBytes)
+
+	// Check if the range is using our custom config.
+	repl := store.LookupReplica(keys.MustAddr(spanConfigTablePrefix))
+	if repl != nil {
+		conf, err := repl.LoadSpanConfig(ctx)
+		require.NoError(t, err)
+		t.Logf("current range config - RangeMaxBytes: %d bytes (%s), "+
+			"RangeMinBytes: %d bytes (%s)",
+			conf.RangeMaxBytes, humanize.Bytes(uint64(conf.RangeMaxBytes)),
+			conf.RangeMinBytes, humanize.Bytes(uint64(conf.RangeMinBytes)))
+
+	}
+
+	t.Logf("targeting span_configurations table at key: %s (table ID %d)\n",
+		spanConfigTablePrefix, keys.SpanConfigurationsTableID)
+
+	// Create a single target for the scratch range (this will be stored in system.span_configurations)
+	scratchTarget := spanconfig.MakeTargetFromSpan(roachpb.Span{
+		Key:    scratchKey,
+		EndKey: scratchKey.PrefixEnd(),
+	})
+
+	// This is a large spanconfig for a scratch range with relevant fields set
+	// to maximum int64 and int32 values. This is done to have a spanconfig that
+	// is large enough to trigger backpressure without having to write a million
+	// records.
+	// We want this config to be relatively large - this is done via setting
+	// values to have max values and multiple fields as this config gets
+	// marshalled into a protobuf and protobuf uses variant encoding, which
+	// means larger values take more bytes to encode.
+	spanConfig2KiB := roachpb.SpanConfig{ // 2078 bytes ~ 2 KiB.
+		RangeMaxBytes: math.MaxInt64,
+		RangeMinBytes: math.MaxInt64,
+		GCPolicy: roachpb.GCPolicy{
+			TTLSeconds: math.MaxInt32,
+			ProtectionPolicies: []roachpb.ProtectionPolicy{
+				{
+					ProtectedTimestamp: hlc.MaxTimestamp,
+				},
+				{
+					ProtectedTimestamp: hlc.MaxTimestamp,
+				},
+			},
+		},
+		NumReplicas: math.MaxInt32,
+		GlobalReads: true,
+		NumVoters:   math.MaxInt32,
+		VoterConstraints: []roachpb.ConstraintsConjunction{
+			{
+				Constraints: []roachpb.Constraint{
+					{Key: "max_key", Value: strings.Repeat("x", 1024)}, // very long constraint value
+				},
+			},
+		},
+		LeasePreferences: []roachpb.LeasePreference{
+			{
+				Constraints: []roachpb.Constraint{
+					{Key: "max_key", Value: strings.Repeat("y", 1024)}, // very long constraint value
+				},
+			},
+		},
+	}
+
+	configBytes, err := protoutil.Marshal(&spanConfig2KiB)
+	require.NoError(t, err)
+
+	require.GreaterOrEqual(t, len(configBytes), 2048,
+		"spanConfig2KiB should be at least 2 KiB in size")
+
+	// Create a record with the span configuration.
+	testRecord, err := spanconfig.MakeRecord(scratchTarget, spanConfig2KiB)
+	require.NoError(t, err)
+
+	// Write span configurations using KVAccessor.
+	// We expect this to fail due to backpressure.
+	var i int
+	for i = 0; i < numWrites; i++ {
+		// Use KVAccessor to update span configurations.
+		err = kvaccessor.UpdateSpanConfigRecords(ctx, nil,
+			[]spanconfig.Record{testRecord}, hlc.MinTimestamp, hlc.MaxTimestamp)
+		if err != nil {
+			break
+		}
+	}
+
+	// Assert that the operation does not fail due to backpressure.
+	require.NoError(t, err,
+		"expected span config writes to not fail due to backpressure, but they did")
+
+	systemSpanConfigurationsTableSpanMVCCStats := roachpb.Span{
+		Key:    keys.SystemSQLCodec.TablePrefix(keys.SpanConfigurationsTableID),
+		EndKey: keys.SystemSQLCodec.TablePrefix(keys.SpanConfigurationsTableID + 1),
+	}
+
+	distSender := s.DistSenderI().(*kvcoord.DistSender)
+
+	// Track aggregate MVCC stats across all SpanConfigurationsTable ranges
+	var aggregateStats enginepb.MVCCStats
+	var rangeCount int
+
+	for key := systemSpanConfigurationsTableSpanMVCCStats.Key; key.Compare(systemSpanConfigurationsTableSpanMVCCStats.EndKey) < 0; {
+		desc, err := distSender.RangeDescriptorCache().Lookup(ctx, keys.MustAddr(key))
+		require.NoError(t, err)
+		d := desc.Desc
+
+		rangeRepl := store.LookupReplica(d.StartKey)
+		if rangeRepl != nil {
+			stats := rangeRepl.GetMVCCStats()
+			aggregateStats.Add(stats)
+			rangeCount++
+		}
+
+		// Move to next range.
+		key = d.EndKey.AsRawKey()
+		if key.Equal(roachpb.KeyMax) {
+			break
+		}
+	}
+
+	require.Greater(t, aggregateStats.Total(), int64(overloadMaxRangeBytes))
+
+	smallSpanConfig := roachpb.SpanConfig{
+		GCPolicy: roachpb.GCPolicy{
+			TTLSeconds: 0,
+		},
+	}
+
+	smallSpanconfigRecord, err := spanconfig.MakeRecord(scratchTarget, smallSpanConfig)
+	require.NoError(t, err)
+
+	smallSpanconfigRecordWriteErr := kvaccessor.UpdateSpanConfigRecords(ctx,
+		[]spanconfig.Target{scratchTarget}, []spanconfig.Record{smallSpanconfigRecord},
+		hlc.MinTimestamp, hlc.MaxTimestamp)
+
+	require.NoError(t, smallSpanconfigRecordWriteErr,
+		"expected smallSpanconfigRecord write to not fail due to backpressure")
+
+}

pkg/kv/kvserver/replica_backpressure.go

@@ -88,9 +88,11 @@ var backpressureByteTolerance = settings.RegisterByteSizeSetting(
 // to be backpressured.
 var backpressurableSpans = []roachpb.Span{
 	{Key: keys.TimeseriesPrefix, EndKey: keys.TimeseriesKeyMax},
-	// Backpressure from the end of the system config forward instead of
-	// over all table data to avoid backpressuring unsplittable ranges.
-	{Key: keys.SystemConfigTableDataMax, EndKey: keys.TableDataMax},
+	// Exclude the span_configurations table to avoid
+	// catch-22 situations where protected timestamp updates or garbage
+	// collection TTL updates are blocked by backpressure.
+	{Key: keys.SystemConfigTableDataMax, EndKey: keys.SpanConfigTableMin},
+	{Key: keys.SpanConfigTableMax, EndKey: keys.TableDataMax},
 }
 
 // canBackpressureBatch returns whether the provided BatchRequest is eligible