backup: add support for locality aware backup compaction #160558

andrew-r-thomas · 2026-01-06T16:38:48Z

This patch teaches backup compactions about locality aware backups, allowing backup compactions to compact locality aware backups, and respect the passed filters.

Fixes: #149593

Release note: None

cockroach-teamcity · 2026-01-06T16:39:00Z

This change is

kev-cao · 2026-01-08T21:42:45Z

pkg/backup/compaction_job.go

 	}

 	var executionLocality roachpb.Locality
 	if options.ExecutionLocality != nil {


Oh this slipped by in a prior review, but we should probably check options.ExecutionLocality.NonEmpty here (in general we should prefer length checks over nil checks for slices).

I'm not seeing options.ExecutionLocality.NonEmpty, this is a tree.Expr that we expect to be a *tree.StrVal, I might be missing something though

Oh whoops that was mb — I was thinking it was a roachpb.Locality. We're good then

kev-cao · 2026-01-08T21:50:48Z

pkg/backup/compaction_processor.go

+	var destLocalityKV string
+	if len(p.spec.URIsByLocalityKV) > 0 {
+		var localitySinkURI string
+		// When matching, more specific KVs in the node locality take precedence
+		// over less specific ones so search back to front.
+		for i := len(p.FlowCtx.EvalCtx.Locality.Tiers) - 1; i >= 0; i-- {
+			tier := p.FlowCtx.EvalCtx.Locality.Tiers[i].String()
+			if dest, ok := p.spec.URIsByLocalityKV[tier]; ok {
+				localitySinkURI = dest
+				destLocalityKV = tier
+				break
+			}
+		}
+		if localitySinkURI != "" {
+			destURI = localitySinkURI
+		} else if p.spec.StrictLocality {
+			// This shouldn't happen unless there was a bug in distsql planning.
+			return errors.Errorf(
+				"sql processor locality %s does not match any of the backup localities %v: cannot proceed with strict locality",
+				p.FlowCtx.EvalCtx.Locality, p.spec.URIsByLocalityKV,
+			)
+		}
+	}
+	defaultConf, err := cloud.ExternalStorageConfFromURI(destURI, user)


nit: I'd like to move this into its own selectLocalityMatchingStorageConf (or whatever name suits your fancy) helper function. Most of the other code in this function has been delegated into separate functions, so I think it'd be good to keep the pattern.

Oh and the nice thing is, since both regular backup and compacted backups share this logic, this gives us a chance to do some code deduping. Less code, best code

kev-cao · 2026-01-08T21:51:22Z

pkg/backup/compaction_processor.go

+			return errors.Errorf(
+				"sql processor locality %s does not match any of the backup localities %v: cannot proceed with strict locality",
+				p.FlowCtx.EvalCtx.Locality, p.spec.URIsByLocalityKV,
+			)


For errors like these, where we're largely doing a sanity check, we can use errors.AssertionFailedf.

kev-cao · 2026-01-08T21:59:30Z

pkg/backup/backupsink/sst_sink_key_writer_test.go

 ) (*SSTSinkKeyWriter, cloud.ExternalStorage) {
 	conf, store := sinkTestSetup(t, settings, elideMode)
-	sink, err := MakeSSTSinkKeyWriter(conf, store)
+	sink, err := MakeSSTSinkKeyWriter(conf, store, "")


nit: one convention we have here is if we provide an r-value to a function like an empty string, boolean, int, etc. we denote the parameter name after the value, so like "" /* localityKV */.

Granted, most IDEs these days can add parameter hints themselves, so I don't think it's necessarily enforced everywhere.

kev-cao · 2026-01-08T22:03:39Z

pkg/sql/distsql_plan_bulk.go

+	if len(instanceIDs) == 0 {
+		err = errors.Newf("no healthy instances found matching filters: %+v", localityFilters)
+	}
+


nit: This feels a little out of scope for this, but I do like the change. Maybe we could move this into its own PR?

How'd you run into this?

So setupAllNodesPlanningSystem didn't previously filter for localities correctly, the gateway node would always be included. This was fine for regular backups as they don't use the instance ids returned from this function, and rely on PartitionSpans to do filtering instead. Compaction doesn't use PartitionSpans, so this seemed like the right spot to get filtered instances.

With that fix, it was possible for us to get an empty list if no instances matched our filters, which we don't check for, and divide by the length, so this seemed like the right spot to throw in a check, happy to move any of that logic around though.

Ah nice catch — looks like we also divide by length in restore too. If it's a bug that's actively blocking this PR, then we can leave it in. Maybe could move it into its own commit, but I don't think it matters too much.

kev-cao · 2026-01-08T22:17:15Z

pkg/backup/compaction_test.go

+		compactedPath := filepath.Join(
+			"incrementals",
+			fullBackupPath,
+			strings.TrimPrefix(end.GoTime().Format(backupbase.DateBasedIncFolderName), "/"),
+		) + "-" + start.GoTime().Format(backupbase.DateBasedIncFolderNameSuffix)


Ughh... I know the reason for this, but doesn't make me hate this any less :(

This could be made a bit more resilient with:

backupbase.DefaultIncrementalsSubdir

backupinfo.ConstructDateBasedIncrementalFolderName

There's also perhaps a route here where instead of constructing these paths ourselves, we can use SHOW BACKUP FILES and call it individually on each locality aware backup.

With some fancy SQL queries, we could determine the exact number of unique data directories and correlate that against the number of expected backups.

Here's a PR of mine where I did that query to find unique data directories:
https://github.com/cockroachdb/cockroach/pull/160027/changes

kev-cao · 2026-01-09T17:15:58Z

pkg/backup/compaction_test.go

+				tempDir,
+				fmt.Sprintf("foo/TestBackupCompactionLocAware/partition-by-unique-key/%d", i),


This patch teaches backup compactions about locality aware backups, allowing backup compactions to compact locality aware backups, and respect the passed filters. Fixes: cockroachdb#149593 Release note: None

andrew-r-thomas force-pushed the compaction-loc-aware branch 5 times, most recently from 846cefe to af26346 Compare January 7, 2026 16:20

andrew-r-thomas marked this pull request as ready for review January 7, 2026 19:01

andrew-r-thomas requested a review from a team as a code owner January 7, 2026 19:01

andrew-r-thomas requested review from dt and removed request for a team and dt January 7, 2026 19:01

andrew-r-thomas force-pushed the compaction-loc-aware branch 2 times, most recently from 63d1625 to ff6b533 Compare January 7, 2026 20:40

andrew-r-thomas requested a review from kev-cao January 7, 2026 23:44

kev-cao reviewed Jan 8, 2026

View reviewed changes

andrew-r-thomas force-pushed the compaction-loc-aware branch from ff6b533 to 7e83650 Compare January 9, 2026 16:05

kev-cao reviewed Jan 9, 2026

View reviewed changes

backup: add support for locality aware backup compaction

4b390fb

This patch teaches backup compactions about locality aware backups, allowing backup compactions to compact locality aware backups, and respect the passed filters. Fixes: cockroachdb#149593 Release note: None

andrew-r-thomas force-pushed the compaction-loc-aware branch from 7e83650 to 4b390fb Compare January 9, 2026 18:03

andrew-r-thomas requested review from dt and jeffswenson and removed request for jeffswenson January 9, 2026 20:55

		tempDir,
		fmt.Sprintf("foo/TestBackupCompactionLocAware/partition-by-unique-key/%d", i),

backup: add support for locality aware backup compaction #160558

Are you sure you want to change the base?

backup: add support for locality aware backup compaction #160558

Conversation

andrew-r-thomas commented Jan 6, 2026

Uh oh!

cockroach-teamcity commented Jan 6, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrew-r-thomas Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

andrew-r-thomas Jan 9, 2026 •

edited

Loading