spanconfig: revisit 3-replica default for keyspace not referenced by SQL zone configs

[tbg]

Is your feature request related to a problem? Please describe.

In https://github.com/cockroachlabs/support/issues/1858 we have a customer who set all of their zone configs to 5x replication. However, spans no longer referenced by a zone config (for example ranges that still haven't merged away - and may not for some time, for maybe weeks) fall back to the default spanconfig which has 3x replication regardless of the default zone config.

This is awkward, since even though these ranges are not referenced by SQL schema, losing availability of them still causes issues. Replication changes on these ranges can leave blocking intents on meta2; metrics will pick up on the unavailability and set off operator alerts, etc.

Ideally there would be a way for the system tenant to specify, in some way, the replication factor for anything not referenced by the SQL zone configs.

Jira issue: CRDB-21168

[tbg]

(for example ranges that still haven't merged away - and may not for some time, for maybe weeks)

Actually they're probably around for good. We have a situation like this:

• r1000 /Table/15-/Table/16 (part of a SQL table, i.e. not guaranteed small)
• r1001 /Table/16-/Table/27 (empty ranges, used to be tables, but now dropped and removed from schema and not referenced in zcfgs)
• r1002 /Table/27-/Table/28 (part of a SQL table, i.e. not guaranteed small)

r1001 won't ever be merged away, as that would require r1000 to be under the size threshold.

[irfansharif]

I'm going to put this on the SQL schema board. When divvying up what parts of the spanconfig package sit where with @ajwerner + friends, the parts implicated here are above KV. Please punt it back if I've gotten it wrong.

[irfansharif]

r1001 won't ever be merged away, as that would require r1000 to be under the size threshold.

It could also be that if the RHS range being merged is "empty" as would be in this example, we ignore the RHS size/threshold since we're not worsening it. Perhaps that's what you were getting at, Tobi?

[tbg]

Ah, so you're saying it should be merged away yet? Yeah, if we have that heuristic that should do it, in theory, but the RHS isn't exactly empty (always some system keys in it that won't go away until GC triggers, but they don't bump the score so that'll never happen)

cockroach/pkg/kv/kvserver/merge_queue.go

Lines 230 to 238 in e822649

	rhsDesc, rhsStats, rhsQPS, rhsQPSOK, err := mq.requestRangeStats(ctx, lhsDesc.EndKey.AsRawKey())
	if err != nil {
	return false, err
	}
	if rhsStats.Total() >= minBytes {
	log.VEventf(ctx, 2, "skipping merge: RHS meets minimum size threshold %d with %d bytes",
	minBytes, rhsStats.Total())
	return false, nil
	}

is what fired above, I think. But even then, if the LHS is just under the split threshold, there is this check we'd hit instead:

cockroach/pkg/kv/kvserver/merge_queue.go

Lines 283 to 292 in e822649

	conservativeLoadBasedSplitThreshold := 0.5 * lhsRepl.SplitByLoadQPSThreshold()
	shouldSplit, _ := shouldSplitRange(ctx, mergedDesc, mergedStats,
	lhsRepl.GetMaxBytes(), lhsRepl.shouldBackpressureWrites(), confReader)
	if shouldSplit || mergedQPS >= conservativeLoadBasedSplitThreshold {
	log.VEventf(ctx, 2,
	"skipping merge to avoid thrashing: merged range %s may split "+
	"(estimated size, estimated QPS: %d, %v)",
	mergedDesc, mergedStats.Total(), mergedQPS)
	return false, nil
	}

[ajwerner]

@tbg can you come back and communicate any sort of priority on this? Nominally we own this, but we don't have a lot of context as a team and we don't have strong opinions. If a speedy outcome is needed, we'll help in any way we can.

[tbg]

No speedy outcome needed, this just doesn't seem quite right. I will assume the customer in https://github.com/cockroachlabs/support/issues/1858 is going to be satisfied once the range has merged away.

[knz]

Here are holes that also need to be considered:

• keyspace of INACTIVE tenant, e.g. during c2c replication
• keyspace of newly created tenant (create_tenant), before the SQL service starts and starts running span generation
• keyspace of newly ingested/imported/restored data

[tbg]

@irfansharif is this issue still current? I think you did something to address it.

[irfansharif]

Ideally there would be a way for the system tenant to specify, in some way, the replication factor for anything not referenced by the SQL zone configs.

In 23.1+ this is now possible using:

 SET CLUSTER SETTING spanconfig.store.fallback_config_override = '
    {
      "gcPolicy": {"ttlSeconds": 3600},
      "numReplicas": 5,
      "rangeMaxBytes": "536870912",
      "rangeMinBytes": "134217728"
    }';

In 22.2, 22.1 it's possible to control num_replicas using the more targeted COCKROACH_FALLBACK_SPANCONFIG_NUM_REPLICAS_OVERRIDE from #93093 + #93092.

Finally, we introduced new "conformance report" code that uses the span config state as authoritative: #90016, addressing #91239. So these kinds of discrepancies, for state no longer referenced by SQL schema, is unlikely to come up anywhere. Closing.