backup: always write slim manifest

Previously, backups would write the slim manifest if the a default true cluster
setting was true. This patch removes this cluster setting, so backups will
always write the slim manifest.

This work is part of an effort to remove deprecated backup manifest versions.
Since this patch will take effect on 25.4 clusters, backup _reads_ on 26.2
clusters can exclusively use the slim manifest without any failback logic to
the deprecated fat manifest.

Since there still exists logic to read the deprecated manifest exclusively
(e.g. SHOW BACKUPS), we cannot remove the deprecated manifest _write_ path
until all these read paths have fallback logic to the new slim manifest-- work
for an upcoming PR.

Informs #139159

Release note: none

Author: msbutler

pkg/backup/backup_job.go

@@ -427,24 +427,16 @@ func backup(
 		}
 	}
 
-	// Write a `BACKUP_MANIFEST` file to support backups in mixed-version clusters
-	// with 22.2 nodes.
-	//
-	// TODO(adityamaru): We can stop writing `BACKUP_MANIFEST` in 23.2
-	// because a mixed-version cluster with 23.1 nodes will read the
-	// `BACKUP_METADATA` instead.
+	// TODO(msbutler): version gate writing the old manifest once we can guarantee
+	// a cluster version that will not read the old manifest.
 	if err := backupinfo.WriteBackupManifest(ctx, defaultStore, backupbase.BackupManifestName,
 		encryption, &kmsEnv, backupManifest); err != nil {
 		return roachpb.RowCount{}, 0, err
 	}
 
-	// Write a `BACKUP_METADATA` file along with SSTs for all the alloc heavy
-	// fields elided from the `BACKUP_MANIFEST`.
-	if backupinfo.WriteMetadataWithExternalSSTsEnabled.Get(&settings.SV) {
-		if err := backupinfo.WriteMetadataWithExternalSSTs(ctx, defaultStore, encryption,
-			&kmsEnv, backupManifest); err != nil {
-			return roachpb.RowCount{}, 0, err
-		}
+	if err := backupinfo.WriteMetadataWithExternalSSTs(ctx, defaultStore, encryption,
+		&kmsEnv, backupManifest); err != nil {
+		return roachpb.RowCount{}, 0, err
 	}
 
 	statsTable := getTableStatsForBackup(ctx, execCtx.ExecCfg().InternalDB.Executor(), backupManifest.Descriptors)

pkg/backup/backup_test.go

@@ -8456,7 +8456,6 @@ func TestIncorrectAccessOfFilesInBackupMetadata(t *testing.T) {
 
 	_, sqlDB, rawDir, cleanupFn := backupRestoreTestSetup(t, singleNode, 1, InitManualReplication)
 	defer cleanupFn()
-	sqlDB.Exec(t, `SET CLUSTER SETTING backup.write_metadata_with_external_ssts.enabled=true`)
 	sqlDB.Exec(t, `CREATE DATABASE r1`)
 	sqlDB.Exec(t, `CREATE TABLE r1.foo ( id INT PRIMARY KEY)`)
 	sqlDB.Exec(t, `INSERT INTO r1.foo VALUES (1)`)
@@ -8502,7 +8501,6 @@ func TestRestoringAcrossVersions(t *testing.T) {
 	tc, sqlDB, rawDir, cleanupFn := backupRestoreTestSetup(t, singleNode, 1, InitManualReplication)
 	defer cleanupFn()
 
-	sqlDB.Exec(t, `SET CLUSTER SETTING backup.write_metadata_with_external_ssts.enabled=true`)
 	sqlDB.Exec(t, `CREATE DATABASE r1`)
 
 	sqlDB.Exec(t, `BACKUP DATABASE r1 INTO 'nodelocal://1/cross_version'`)
@@ -8615,7 +8613,6 @@ func TestManifestBitFlip(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 	_, sqlDB, rawDir, cleanupFn := backupRestoreTestSetup(t, singleNode, 1, InitManualReplication)
-	sqlDB.Exec(t, `SET CLUSTER SETTING backup.write_metadata_with_external_ssts.enabled=true`)
 	defer cleanupFn()
 	sqlDB.Exec(t, `CREATE DATABASE r1; CREATE DATABASE r2; CREATE DATABASE r3;`)
 	const checksumError = "checksum mismatch"

pkg/backup/backupbase/constants.go

@@ -38,8 +38,8 @@ const (
 	// BackupManifestName is the file name used for serialized BackupManifest
 	// protos.
 	//
-	// TODO(adityamaru): Remove in 23.2 since at that point all nodes will be
-	// writing a SlimBackupManifest instead.
+	// TODO(msbutler): Remove 26.3 when we're guaranteed that no backup wrote
+	// exclusively the backup_manifest, and not the slim manifest.
 	BackupManifestName = "BACKUP_MANIFEST"
 
 	// BackupMetadataName is the file name used for serialized BackupManifest

pkg/backup/backupinfo/BUILD.bazel

@@ -24,7 +24,6 @@ go_library(
         "//pkg/kv/kvpb",
         "//pkg/roachpb",
         "//pkg/security/username",
-        "//pkg/settings",
         "//pkg/sql",
         "//pkg/sql/catalog",
         "//pkg/sql/catalog/dbdesc",
@@ -44,7 +43,6 @@ go_library(
         "//pkg/util/hlc",
         "//pkg/util/ioctx",
         "//pkg/util/log",
-        "//pkg/util/metamorphic",
         "//pkg/util/mon",
         "//pkg/util/protoutil",
         "//pkg/util/syncutil",

pkg/backup/backupinfo/manifest_handling.go

@@ -29,7 +29,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/kv/kvpb"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
-	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc"
@@ -48,7 +47,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
 	"github.com/cockroachdb/cockroach/pkg/util/ioctx"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
-	"github.com/cockroachdb/cockroach/pkg/util/metamorphic"
 	"github.com/cockroachdb/cockroach/pkg/util/mon"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
@@ -86,17 +84,6 @@ const (
 	BackupProgressDirectory = "progress"
 )
 
-// WriteMetadataWithExternalSSTsEnabled controls if we write a `BACKUP_METADATA`
-// file along with external SSTs containing lists of `BackupManifest_Files` and
-// descriptors. This new format of metadata is written in addition to the
-// `BACKUP_MANIFEST` file, and is expected to be its replacement in the future.
-var WriteMetadataWithExternalSSTsEnabled = settings.RegisterBoolSetting(
-	settings.ApplicationLevel,
-	"backup.write_metadata_with_external_ssts.enabled",
-	"write BACKUP metadata along with supporting SST files",
-	metamorphic.ConstantWithTestBool("backup.write_metadata_with_external_ssts.enabled", true),
-)
-
 // IsGZipped detects whether the given bytes represent GZipped data. This check
 // is used rather than a standard implementation such as http.DetectContentType
 // since some zipped data may be mis-identified by that method. We've seen

pkg/backup/compaction_job.go

@@ -797,11 +797,9 @@ func concludeBackupCompaction(
 		details.EncryptionOptions, kmsEnv, backupManifest); err != nil {
 		return err
 	}
-	if backupinfo.WriteMetadataWithExternalSSTsEnabled.Get(&execCtx.ExecCfg().Settings.SV) {
-		if err := backupinfo.WriteMetadataWithExternalSSTs(ctx, store, details.EncryptionOptions,
-			kmsEnv, backupManifest); err != nil {
-			return err
-		}
+	if err := backupinfo.WriteMetadataWithExternalSSTs(ctx, store, details.EncryptionOptions,
+		kmsEnv, backupManifest); err != nil {
+		return err
 	}
 
 	statsTable := getTableStatsForBackup(ctx, execCtx.ExecCfg().InternalDB.Executor(), backupManifest.Descriptors)

pkg/backup/compaction_test.go

@@ -16,7 +16,6 @@ import (
 	"testing"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/backup/backupinfo"
 	"github.com/cockroachdb/cockroach/pkg/backup/backuppb"
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/build"
@@ -47,11 +46,9 @@ func TestBackupCompaction(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	ctx := context.Background()
 	tempDir, tempDirCleanup := testutils.TempDir(t)
 	defer tempDirCleanup()
 	st := cluster.MakeTestingClusterSettings()
-	backupinfo.WriteMetadataWithExternalSSTsEnabled.Override(ctx, &st.SV, true)
 	_, db, cleanupDB := backupRestoreTestSetupEmpty(
 		t, singleNode, tempDir, InitManualReplication, base.TestClusterArgs{
 			ServerArgs: base.TestServerArgs{