kvserver: don't apply ExcludeDataFromBackup outside config bounds

[stevendanna]

Some span configuration updates result in a replica split. However, between the new span configuration being set on the replica and the split happening, the span configuration stored on the replica may be incorrect for a portion of the replica's keyspace.

This is a general problem with asyncronous span configuration application. But, here we introduce a specific fix for the ExcludeDataFromBackup span configuration item.

When setting the span configuration, we now also store the bounds of that span configuration. ExcludeDataFromBackup only returns true if the data being considered for exclusion is contained by those bounds.

Further, we only set DataExcludedFromBackup to true in
BatchTimestampBeforeGCError if the span of the batch that generated
the error is contained by the span configuration bound. This field is
used to ignore BatchTimestampBeforeGCErrors. As a result, not setting
it to true could result in BACKUP failures until the range
splits. However, the alternative is silently ignoring a possible real
error.

Note that this doesn't solve all problems related to DataExcludedFromBackup. For instance, span configuration propagation is asyncronous and thus data for a given table may continue to be excluded from backups for some period after ALTER TABLE <TABLE> SET (exclude_data_from_backup = false) has been set.

Fixes #120122

Release note (bug fix): Fix a rare condition in which a BACKUP taken shortly after ALTER TABLE

SET (exclude_data_from_backup = true) could result in data from an unrelated table being excluded from a backup.

[cockroach-teamcity]

This change is 

[stevendanna]

Suggested change

	// GetSpanConfigForKeyWithBounds is added for the spanconfig.StoreReader interface, required for
	// SpanConfigConformanceReport.
	// GetSpanConfigForKeyWithBounds is added for the spanconfig.StoreReader interface.

[stevendanna]

Suggested change

	// GetSpanConfigForKeyWithBounds looks of the span config for the given
	// key. It's part of spanconfig.StoreReader interface. Note that it is only
	// usable for the system tenant config.
	// GetSpanConfigForKeyWithBounds returns the span config for the given
	// key and the bounds that span the configuration applies to. It's part of spanconfig.StoreReader interface. Note that it is only
	// usable for the system tenant config.

[arulajmani]

modulo nits

Reviewed 25 of 25 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dt and @stevendanna)

---

pkg/config/system.go line 348 at r1 (raw file):

Previously, stevendanna (Steven Danna) wrote…

// GetSpanConfigForKeyWithBounds returns the span config for the given
// key and the bounds that span the configuration applies to. It's part of spanconfig.StoreReader interface. Note that it is only
// usable for the system tenant config.

Updated comment looks good to me

---

pkg/kv/kvserver/replica.go line 1230 at r1 (raw file):

func (r *Replica) excludeReplicaFromBackupRLocked(rspan roachpb.RSpan) bool {
	if r.mu.conf.ExcludeDataFromBackup {
		// If ExcludeDataFromBackup is set, we also want to ensure that

nit: consider de-duplicating some of the logic shared here and above.

---

pkg/spanconfig/spanconfig.go line 292 at r1 (raw file):

	ComputeSplitKey(ctx context.Context, start, end roachpb.RKey) (roachpb.RKey, error)
	GetSpanConfigForKey(ctx context.Context, key roachpb.RKey) (roachpb.SpanConfig, error)
	GetSpanConfigForKeyWithBounds(ctx context.Context, key roachpb.RKey) (roachpb.SpanConfig, roachpb.Span, error)

nit: instead of having these two methods, should we instead have GetSpanConfigForKey return the span it applies to as well? Callers that don't care about the span are free to ignore it.

Separately, consider adding a comment here and/or explicitly naming the second return value to something like "appliesTo".

[stevendanna]

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @arulajmani and @dt)

---

pkg/kv/kvserver/replica.go line 1230 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: consider de-duplicating some of the logic shared here and above.

Done.

---

pkg/spanconfig/spanconfig.go line 292 at r1 (raw file):

Previously, arulajmani (Arul Ajmani) wrote…

nit: instead of having these two methods, should we instead have GetSpanConfigForKey return the span it applies to as well? Callers that don't care about the span are free to ignore it.

Separately, consider adding a comment here and/or explicitly naming the second return value to something like "appliesTo".

Done and done.

[stevendanna]

bors r=arulajmani

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)
• check_generated_code
• docker_image_amd64
• examples_orms
• macos_amd64_build
• macos_arm64_build
• windows_build