Merge pull request cockroachdb#80670 from adityamaru/backport22.1.0-8…

…0587 release-22.1.0: backupccl: allow secondary tenants to write pts records during backup
andreimatei · Apr 28, 2022 · bbeaf44 · bbeaf44
2 parents 4948a17 + 84a3833
commit bbeaf44
Show file tree

Hide file tree

Showing 7 changed files with 277 additions and 206 deletions.
diff --git a/pkg/ccl/backupccl/backup_job.go b/pkg/ccl/backupccl/backup_job.go
@@ -19,6 +19,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/joberror"
@@ -413,18 +414,35 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 			return err
 		}
 		details = backupDetails
+		// Reset backupDetails so nobody accidentally uses it.
+		backupDetails = jobspb.BackupDetails{}
 		backupManifest = &m
 
-		if len(backupManifest.Spans) > 0 && p.ExecCfg().Codec.ForSystemTenant() {
-			protectedtsID := uuid.MakeV4()
-			details.ProtectedTimestampRecord = &protectedtsID
+		// Now that we have resolved the details, and manifest, write a protected
+		// timestamp record on the backup's target spans/schema object.
+		//
+		// This closure updates `details` to store the protected timestamp records
+		// UUID so that it can be released on job completion. The updated details
+		// are persisted in the job record further down.
+		{
+			if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.EnableProtectedTimestampsForTenant) {
+				protectedtsID := uuid.MakeV4()
+				details.ProtectedTimestampRecord = &protectedtsID
+			} else if len(backupManifest.Spans) > 0 && p.ExecCfg().Codec.ForSystemTenant() {
+				// Prior to clusterversion.EnableProtectedTimestampsForTenant only the
+				// system tenant can write a protected timestamp record.
+				protectedtsID := uuid.MakeV4()
+				details.ProtectedTimestampRecord = &protectedtsID
+			}
 
-			if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
-				return protectTimestampForBackup(
-					ctx, p.ExecCfg(), txn, b.job.ID(), m, details,
-				)
-			}); err != nil {
-				return err
+			if details.ProtectedTimestampRecord != nil {
+				if err := p.ExecCfg().DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) error {
+					return protectTimestampForBackup(
+						ctx, p.ExecCfg(), txn, b.job.ID(), m, details,
+					)
+				}); err != nil {
+					return err
+				}
 			}
 		}
 

diff --git a/pkg/ccl/backupccl/backup_planning.go b/pkg/ccl/backupccl/backup_planning.go
@@ -920,7 +920,10 @@ func backupPlanHook(
 			return err
 		}
 
-		if len(backupManifest.Spans) > 0 && p.ExecCfg().Codec.ForSystemTenant() {
+		if p.ExecCfg().Settings.Version.IsActive(ctx, clusterversion.EnableProtectedTimestampsForTenant) {
+			protectedtsID := uuid.MakeV4()
+			backupDetails.ProtectedTimestampRecord = &protectedtsID
+		} else if len(backupManifest.Spans) > 0 && p.ExecCfg().Codec.ForSystemTenant() {
 			protectedtsID := uuid.MakeV4()
 			backupDetails.ProtectedTimestampRecord = &protectedtsID
 		}
@@ -945,10 +948,12 @@ func backupPlanHook(
 			p.ExecCfg().Settings, p.ExecCfg().LogicalClusterID(), p.ExecCfg().Organization(), "",
 		) != nil
 
-		if err := protectTimestampForBackup(
-			ctx, p.ExecCfg(), plannerTxn, jobID, backupManifest, backupDetails,
-		); err != nil {
-			return err
+		if backupDetails.ProtectedTimestampRecord != nil {
+			if err := protectTimestampForBackup(
+				ctx, p.ExecCfg(), plannerTxn, jobID, backupManifest, backupDetails,
+			); err != nil {
+				return err
+			}
 		}
 
 		if backupStmt.Options.Detached {
@@ -1452,32 +1457,23 @@ func protectTimestampForBackup(
 	backupManifest BackupManifest,
 	backupDetails jobspb.BackupDetails,
 ) error {
-	if backupDetails.ProtectedTimestampRecord == nil {
-		return nil
-	}
-	if len(backupManifest.Spans) > 0 {
-		tsToProtect := backupManifest.EndTime
-		if !backupManifest.StartTime.IsEmpty() {
-			tsToProtect = backupManifest.StartTime
-		}
-
-		// Resolve the target that the PTS record will protect as part of this
-		// backup.
-		target := getProtectedTimestampTargetForBackup(backupManifest)
-
-		// Records written by the backup job should be ignored when making GC
-		// decisions on any table that has been marked as
-		// `exclude_data_from_backup`. This ensures that the backup job does not
-		// holdup GC on that table span for the duration of execution.
-		target.IgnoreIfExcludedFromBackup = true
-		rec := jobsprotectedts.MakeRecord(*backupDetails.ProtectedTimestampRecord, int64(jobID),
-			tsToProtect, backupManifest.Spans, jobsprotectedts.Jobs, target)
-		err := execCfg.ProtectedTimestampProvider.Protect(ctx, txn, rec)
-		if err != nil {
-			return err
-		}
-	}
-	return nil
+	tsToProtect := backupManifest.EndTime
+	if !backupManifest.StartTime.IsEmpty() {
+		tsToProtect = backupManifest.StartTime
+	}
+
+	// Resolve the target that the PTS record will protect as part of this
+	// backup.
+	target := getProtectedTimestampTargetForBackup(backupManifest)
+
+	// Records written by the backup job should be ignored when making GC
+	// decisions on any table that has been marked as
+	// `exclude_data_from_backup`. This ensures that the backup job does not
+	// holdup GC on that table span for the duration of execution.
+	target.IgnoreIfExcludedFromBackup = true
+	rec := jobsprotectedts.MakeRecord(*backupDetails.ProtectedTimestampRecord, int64(jobID),
+		tsToProtect, backupManifest.Spans, jobsprotectedts.Jobs, target)
+	return execCfg.ProtectedTimestampProvider.Protect(ctx, txn, rec)
 }
 
 func getEncryptedDataKeyFromURI(

diff --git a/pkg/ccl/backupccl/backup_tenant_test.go b/pkg/ccl/backupccl/backup_tenant_test.go
@@ -122,7 +122,7 @@ func TestTenantBackupMultiRegionDatabases(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
-	skip.UnderStressRace(t, "times out")
+	skip.UnderStressRace(t, "test is too heavy to run under stress")
 
 	tc, db, cleanup := multiregionccltestutils.TestingCreateMultiRegionCluster(
 		t, 3 /*numServers*/, base.TestingKnobs{},
@@ -138,36 +138,14 @@ func TestTenantBackupMultiRegionDatabases(t *testing.T) {
 	defer tSQL.Close()
 	tenSQLDB := sqlutils.MakeSQLRunner(tSQL)
 
-	waitForSettingToTakeEffect := func(expValue string) {
-		testutils.SucceedsSoon(t, func() error {
-			var val string
-			tenSQLDB.QueryRow(t,
-				fmt.Sprintf(
-					"SHOW CLUSTER SETTING %s", sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
-				),
-			).Scan(&val)
-
-			if val != expValue {
-				return errors.Newf("waiting for cluster setting to be set to %q", expValue)
-			}
-			return nil
-		})
-	}
-
-	setTenantReadOnlyClusterSetting := func(val string) {
-		sqlDB.Exec(
-			t,
-			fmt.Sprintf(
-				"ALTER TENANT $1 SET CLUSTER	SETTING %s = '%s'",
-				sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
-				val,
-			),
-			tenID.ToUint64(),
-		)
-	}
-
-	setTenantReadOnlyClusterSetting("true")
-	waitForSettingToTakeEffect("true")
+	setAndWaitForTenantReadOnlyClusterSetting(
+		t,
+		sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+		sqlDB,
+		tenSQLDB,
+		tenID,
+		"true",
+	)
 
 	// Setup.
 	const tenDst = "userfile:///ten_backup"
@@ -181,8 +159,14 @@ func TestTenantBackupMultiRegionDatabases(t *testing.T) {
 	{
 		// Flip the tenant-read only cluster setting; ensure database can be restored
 		// on the system tenant but not on the secondary tenant.
-		setTenantReadOnlyClusterSetting("false")
-		waitForSettingToTakeEffect("false")
+		setAndWaitForTenantReadOnlyClusterSetting(
+			t,
+			sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+			sqlDB,
+			tenSQLDB,
+			tenID,
+			"false",
+		)
 
 		tenSQLDB.Exec(t, "DROP DATABASE mrdb CASCADE")
 		tenSQLDB.ExpectErr(
@@ -199,8 +183,14 @@ func TestTenantBackupMultiRegionDatabases(t *testing.T) {
 	{
 		// Flip the tenant-read only cluster setting back to true and ensure the
 		// restore succeeds.
-		setTenantReadOnlyClusterSetting("true")
-		waitForSettingToTakeEffect("true")
+		setAndWaitForTenantReadOnlyClusterSetting(
+			t,
+			sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+			sqlDB,
+			tenSQLDB,
+			tenID,
+			"true",
+		)
 
 		tenSQLDB.Exec(t, fmt.Sprintf("RESTORE DATABASE mrdb FROM LATEST IN '%s'", tenDst))
 	}
@@ -214,8 +204,14 @@ func TestTenantBackupMultiRegionDatabases(t *testing.T) {
 				"SET CLUSTER SETTING %s = 'us-east1'", sql.DefaultPrimaryRegionClusterSettingName,
 			),
 		)
-		setTenantReadOnlyClusterSetting("false")
-		waitForSettingToTakeEffect("false")
+		setAndWaitForTenantReadOnlyClusterSetting(
+			t,
+			sql.SecondaryTenantsMultiRegionAbstractionsEnabledSettingName,
+			sqlDB,
+			tenSQLDB,
+			tenID,
+			"false",
+		)
 
 		tenSQLDB.Exec(t, "CREATE DATABASE nonMrDB")
 		tenSQLDB.Exec(t, fmt.Sprintf("BACKUP DATABASE nonMrDB INTO '%s'", tenDst))