Merge #114464 #118950

114464: backupccl: allow online restore to assume exclusive end keys r=dt a=stevendanna

Currently, we only support non-revision-history restores for online restore. As a result, we can assume file span end keys are exclusive. We were implicitly assuming this and throwing away files that didn't overlap with the restore span entry.

Epic: none

Release note: None

118950: workflows: shut down bazel server if we're asked to cancel the build r=rail a=rickystewart

This is implemented as a [SIGINT](https://docs.github.com/en/actions/managing-workflow-runs/canceling-a-workflow#steps-github-takes-to-cancel-a-workflow-run). However, the build is likely to just continue running in the background because this may not be enough to tear Bazel down. This could cause steps that need to happen later to stall indefinitely. We just shut down the server directly in this case.

Epic: CRDB-8308
Release note: None

Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>

Author: 3 people

.github/workflows/experimental-github-actions-testing.yml

@@ -17,7 +17,7 @@ jobs:
           ref: ${{ github.event.pull_request.head.sha }}
       - run: ./build/github/get-engflow-keys.sh
       - name: run tests
-        run: bazel test //pkg:all_tests //pkg/ui:lint //pkg/ui:test --config crosslinux --jobs 300 --remote_download_minimal --bes_keywords=github_pr_number=${{ github.event.pull_request.number }} --config=use_ci_timeouts --build_event_binary_file=bes.bin $(./build/github/engflow-args.sh) --bes_keywords ci-unit-test
+        run: ./build/github/run-bazel.sh test //pkg:all_tests //pkg/ui:lint //pkg/ui:test --config crosslinux --jobs 300 --remote_download_minimal --bes_keywords=github_pr_number=${{ github.event.pull_request.number }} --config=use_ci_timeouts --build_event_binary_file=bes.bin $(./build/github/engflow-args.sh) --bes_keywords ci-unit-test
       - name: upload test results
         run: ./build/github/summarize-build.sh bes.bin
         if: always()

build/github/run-bazel.sh

@@ -0,0 +1,11 @@
+# Any arguments passed to this script will be directly passed to Bazel. In
+# addition to just calling Bazel, this script has the property that the Bazel
+# server will be killed if we receive an interrupt.
+
+bazel_shutdown() {
+    bazel shutdown
+}
+
+trap bazel_shutdown EXIT
+
+bazel "$@"

pkg/ccl/backupccl/bench_covering_test.go

@@ -111,6 +111,7 @@ func BenchmarkRestoreEntryCover(b *testing.B) {
 													layerToBackupManifestFileIterFactory,
 													nil,
 													filter,
+													&inclusiveEndKeyComparator{},
 													spanCh)
 											})
 

pkg/ccl/backupccl/generative_split_and_scatter_processor.go

@@ -476,13 +476,20 @@ func runGenerativeSplitAndScatter(
 			return errors.Wrap(err, "failed to make span covering filter")
 		}
 		defer filter.close()
+
+		var fsc fileSpanComparator = &inclusiveEndKeyComparator{}
+		if spec.ExclusiveFileSpanComparison {
+			fsc = &exclusiveEndKeyComparator{}
+		}
+
 		return errors.Wrap(generateAndSendImportSpans(
 			ctx,
 			spec.Spans,
 			backups,
 			layerToFileIterFactory,
 			backupLocalityMap,
 			filter,
+			fsc,
 			restoreSpanEntriesCh,
 		), "generating and sending import spans")
 	})

pkg/ccl/backupccl/restore_job.go

@@ -344,6 +344,14 @@ func restore(
 		return roachpb.RowCount{}, err
 	}
 
+	var fsc fileSpanComparator = &inclusiveEndKeyComparator{}
+	// We know we can use exclusive comparison for
+	// ExperimentalOnline since we don't support revision history
+	// backups for ExperimentalOnline.
+	if details.ExperimentalOnline {
+		fsc = &exclusiveEndKeyComparator{}
+	}
+
 	countSpansCh := make(chan execinfrapb.RestoreSpanEntry, 1000)
 	genSpan := func(ctx context.Context, spanCh chan execinfrapb.RestoreSpanEntry) error {
 		defer close(spanCh)
@@ -354,6 +362,7 @@ func restore(
 			layerToIterFactory,
 			backupLocalityMap,
 			filter,
+			fsc,
 			spanCh,
 		), "generate and send import spans")
 	}

pkg/ccl/backupccl/restore_online.go

@@ -173,14 +173,16 @@ func sendAddRemoteSSTWorker(
 			firstSplitDone := false
 			for _, file := range entry.Files {
 				restoringSubspan := file.BackupFileEntrySpan.Intersect(entry.Span)
-				log.Infof(ctx, "Experimental restore: sending span %s of file (path: %s, span: %s), with intersecting subspan %s",
-					file.BackupFileEntrySpan, file.Path, file.BackupFileEntrySpan, restoringSubspan)
-
 				if !restoringSubspan.Valid() {
-					log.Warningf(ctx, "backup file does not intersect with the restoring span")
-					continue
+					return errors.AssertionFailedf("file %s with span %s has no overlap with restore span %s",
+						file.Path,
+						file.BackupFileEntrySpan,
+						entry.Span,
+					)
 				}
 
+				log.Infof(ctx, "experimental restore: sending span %s of file %s (file span: %s) as part of restore span %s",
+					restoringSubspan, file.Path, file.BackupFileEntrySpan, entry.Span)
 				file.BackupFileEntrySpan = restoringSubspan
 				if !firstSplitDone {
 					expiration := execCtx.ExecCfg().Clock.Now().AddDuration(time.Hour)

pkg/ccl/backupccl/restore_span_covering.go

@@ -235,10 +235,9 @@ func (f spanCoveringFilter) close() {
 // trimmed/removed based on the lowWaterMark before the covering for them is
 // generated. These spans are generated one at a time and then sent to spanCh.
 //
-// Note that because of https://github.com/cockroachdb/cockroach/issues/101963,
-// the spans of files are end key _inclusive_. Because the current definition
-// of spans are all end key _exclusive_, we work around this by assuming that
-// the end key of each file span is actually the next key of the end key.
+// Spans are considered for inclusion using the fileSpanComparator. Note that
+// because of https://github.com/cockroachdb/cockroach/issues/101963, most
+// backups use a comparator that treats a file's end key as _inclusive_.
 //
 // Consider a chain of backups with files f1, f2… which cover spans as follows:
 //
@@ -279,6 +278,7 @@ func generateAndSendImportSpans(
 	layerToBackupManifestFileIterFactory backupinfo.LayerToBackupManifestFileIterFactory,
 	backupLocalityMap map[int]storeByLocalityKV,
 	filter spanCoveringFilter,
+	fsc fileSpanComparator,
 	spanCh chan execinfrapb.RestoreSpanEntry,
 ) error {
 
@@ -378,7 +378,7 @@ func generateAndSendImportSpans(
 					coverSpan.EndKey = span.EndKey
 				}
 
-				newFilesByLayer, err := getNewIntersectingFilesByLayer(coverSpan, layersCoveredLater, fileIterByLayer)
+				newFilesByLayer, err := getNewIntersectingFilesByLayer(coverSpan, layersCoveredLater, fileIterByLayer, fsc)
 				if err != nil {
 					return err
 				}
@@ -405,7 +405,7 @@ func generateAndSendImportSpans(
 							sz = 16 << 20
 						}
 
-						if inclusiveOverlap(coverSpan, file.Span) {
+						if fsc.overlaps(coverSpan, file.Span) {
 							covSize += sz
 							filesByLayer[layer] = append(filesByLayer[layer], file)
 						}
@@ -608,6 +608,7 @@ func getNewIntersectingFilesByLayer(
 	span roachpb.Span,
 	layersCoveredLater map[int]bool,
 	fileIters []bulk.Iterator[*backuppb.BackupManifest_File],
+	fsc fileSpanComparator,
 ) ([][]*backuppb.BackupManifest_File, error) {
 	var files [][]*backuppb.BackupManifest_File
 
@@ -629,7 +630,7 @@ func getNewIntersectingFilesByLayer(
 				// inclusive. Because roachpb.Span and its associated operations
 				// are end key exclusive, we work around this by replacing the
 				// end key with its next value in order to include the end key.
-				if inclusiveOverlap(span, f.Span) {
+				if fsc.overlaps(span, f.Span) {
 					layerFiles = append(layerFiles, f)
 				}
 
@@ -644,8 +645,23 @@ func getNewIntersectingFilesByLayer(
 	return files, nil
 }
 
-// inclusiveOverlap returns true if sp, which is end key exclusive, overlaps
-// isp, which is end key inclusive.
-func inclusiveOverlap(sp roachpb.Span, isp roachpb.Span) bool {
-	return sp.Overlaps(isp) || sp.ContainsKey(isp.EndKey)
+type fileSpanComparator interface {
+	overlaps(targetSpan, fileSpan roachpb.Span) bool
+}
+
+// inclusiveEndKeyComparator assumes that file spans have inclusive
+// end keys.
+type inclusiveEndKeyComparator struct{}
+
+func (*inclusiveEndKeyComparator) overlaps(targetSpan, inclusiveEndKeyFileSpan roachpb.Span) bool {
+	// TODO(ssd): Could ContainsKey here be replaced with targetSpan.Key.Equal(inclusiveEndKeyFileSpan.EndKey)?
+	return targetSpan.Overlaps(inclusiveEndKeyFileSpan) || targetSpan.ContainsKey(inclusiveEndKeyFileSpan.EndKey)
+}
+
+// exclusiveEndKeyComparator assumes that file spans have exclusive
+// end keys.
+type exclusiveEndKeyComparator struct{}
+
+func (*exclusiveEndKeyComparator) overlaps(targetSpan, inclusiveEndKeyFileSpan roachpb.Span) bool {
+	return targetSpan.Overlaps(inclusiveEndKeyFileSpan)
 }

pkg/ccl/backupccl/restore_span_covering_test.go

@@ -301,6 +301,7 @@ func makeImportSpans(
 		layerToIterFactory,
 		nil,
 		filter,
+		&inclusiveEndKeyComparator{},
 		spanCh)
 	close(spanCh)
 

pkg/sql/execinfrapb/processors_bulk_io.proto

@@ -323,7 +323,7 @@ message RestoreFileSpec {
   optional roachpb.RowCount backup_file_entry_counts = 6 [(gogoproto.nullable) = false];
   optional uint64 backing_file_size = 7 [(gogoproto.nullable) = false];
   optional uint64 approximate_physical_size = 8 [(gogoproto.nullable) = false];
-  
+
   // NEXT ID: 9.
 }
 
@@ -447,6 +447,9 @@ message GenerativeSplitAndScatterSpec {
   optional int64 job_id = 18 [(gogoproto.nullable) = false, (gogoproto.customname) = "JobID"];
   optional bool use_frontier_checkpointing = 20 [(gogoproto.nullable) = false];
   repeated jobs.jobspb.RestoreProgress.FrontierEntry checkpointed_spans = 21 [(gogoproto.nullable) = false];
+  // ExclusiveFileSpanComparison is true if the backup can safely use
+  // exclusive file span comparison.
+  optional bool exclusive_file_span_comparison = 22 [(gogoproto.nullable) = false];
 
   reserved 19;
 }