Incremental backup: do not error on empty backup

go/test/endtoend/backup/vtctlbackup/backup_utils.go

@@ -1255,8 +1255,9 @@ func waitForNumBackups(t *testing.T, expectNumBackups int) []string {
 	}
 }
 
-func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos string, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
+func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos string, expectEmpty bool, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
 	numBackups := len(waitForNumBackups(t, -1))
+
 	output, err := localCluster.VtctldClientProcess.ExecuteCommandWithOutput("Backup", "--incremental-from-pos", incrementalFromPos, replica.Alias)
 	if expectError != "" {
 		require.Errorf(t, err, "expected: %v", expectError)
@@ -1265,18 +1266,24 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre
 	}
 	require.NoErrorf(t, err, "output: %v", output)
 
+	if expectEmpty {
+		require.Contains(t, output, mysqlctl.EmptyBackupMessage)
+		return nil, ""
+	}
+
 	backups := waitForNumBackups(t, numBackups+1)
 	require.NotEmptyf(t, backups, "output: %v", output)
 
 	verifyTabletBackupStats(t, replica.VttabletProcess.GetVars())
 	backupName = backups[len(backups)-1]
+
 	backupLocation := localCluster.CurrentVTDATAROOT + "/backups/" + shardKsName + "/" + backupName
 	return readManifestFile(t, backupLocation), backupName
 }
 
-func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos string, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
+func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos string, expectEmpty bool, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
 	replica := getReplica(t, replicaIndex)
-	return testReplicaIncrementalBackup(t, replica, incrementalFromPos, expectError)
+	return testReplicaIncrementalBackup(t, replica, incrementalFromPos, expectEmpty, expectError)
 }
 
 func TestReplicaFullRestore(t *testing.T, replicaIndex int, expectError string) {

go/test/endtoend/backup/vtctlbackup/pitr_test_framework.go

@@ -138,6 +138,7 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase)
 			name              string
 			writeBeforeBackup bool
 			fromFullPosition  bool
+			expectEmpty       bool
 			incrementalFrom   incrementalFromPosType
 			expectError       string
 		}{
@@ -146,24 +147,29 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase)
 				incrementalFrom: incrementalFromPosPosition,
 			},
 			{
-				name:            "fail1",
+				name:            "empty1",
 				incrementalFrom: incrementalFromPosPosition,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
-				name:            "fail2",
+				name:            "empty2",
+				incrementalFrom: incrementalFromPosAuto,
+				expectEmpty:     true,
+			},
+			{
+				name:            "empty3",
 				incrementalFrom: incrementalFromPosPosition,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
 				name:              "make writes, succeed",
 				writeBeforeBackup: true,
 				incrementalFrom:   incrementalFromPosPosition,
 			},
 			{
-				name:            "fail, no binary logs to backup",
+				name:            "empty again",
 				incrementalFrom: incrementalFromPosPosition,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
 				name:              "make writes again, succeed",
@@ -176,9 +182,9 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase)
 				incrementalFrom:   incrementalFromPosAuto,
 			},
 			{
-				name:            "fail auto position, no binary logs to backup",
+				name:            "empty again, based on auto position",
 				incrementalFrom: incrementalFromPosAuto,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
 				name:              "auto position, make writes again, succeed",
@@ -223,10 +229,15 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase)
 					}
 				}
 				// always use same 1st replica
-				manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectError)
+				manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectEmpty, tc.expectError)
 				if tc.expectError != "" {
 					return
 				}
+				if tc.expectEmpty {
+					assert.Nil(t, manifest)
+					return
+				}
+				require.NotNil(t, manifest)
 				defer func() {
 					lastBackupPos = manifest.Position
 					lastBackupName = manifest.BackupName
@@ -352,6 +363,7 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 			name              string
 			writeBeforeBackup bool
 			fromFullPosition  bool
+			expectEmpty       bool
 			incrementalFrom   incrementalFromPosType
 			expectError       string
 		}{
@@ -360,24 +372,29 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 				incrementalFrom: incrementalFromPosPosition,
 			},
 			{
-				name:            "fail1",
+				name:            "empty1",
 				incrementalFrom: incrementalFromPosPosition,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
-				name:            "fail2",
+				name:            "empty2",
+				incrementalFrom: incrementalFromPosAuto,
+				expectEmpty:     true,
+			},
+			{
+				name:            "empty3",
 				incrementalFrom: incrementalFromPosPosition,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
 				name:              "make writes, succeed",
 				writeBeforeBackup: true,
 				incrementalFrom:   incrementalFromPosPosition,
 			},
 			{
-				name:            "fail, no binary logs to backup",
+				name:            "empty again",
 				incrementalFrom: incrementalFromPosPosition,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
 				name:              "make writes again, succeed",
@@ -390,9 +407,9 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 				incrementalFrom:   incrementalFromPosAuto,
 			},
 			{
-				name:            "fail auto position, no binary logs to backup",
+				name:            "empty again, based on auto position",
 				incrementalFrom: incrementalFromPosAuto,
-				expectError:     "no binary logs to backup",
+				expectEmpty:     true,
 			},
 			{
 				name:              "auto position, make writes again, succeed",
@@ -434,11 +451,16 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 						incrementalFromPos = replication.EncodePosition(fullBackupPos)
 					}
 				}
-				manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectError)
+				manifest, backupName := TestReplicaIncrementalBackup(t, 0, incrementalFromPos, tc.expectEmpty, tc.expectError)
 				if tc.expectError != "" {
 					return
 				}
-				// We wish to mark the current post-backup timestamp. We will later on retore to this point in time.
+				if tc.expectEmpty {
+					assert.Nil(t, manifest)
+					return
+				}
+				require.NotNil(t, manifest)
+				// We wish to mark the current post-backup timestamp. We will later on restore to this point in time.
 				// However, the restore is up to and _exclusive_ of the timestamp. So for test's sake, we sleep
 				// an extra few milliseconds just to ensure the timestamp we read is strictly after the backup time.
 				// This is basicaly to avoid weird flakiness in CI.
@@ -707,10 +729,11 @@ func ExecTestIncrementalBackupOnTwoTablets(t *testing.T, tcase *PITRTestCase) {
 
 						lastBackupPos = fullBackupPos
 					case operationIncrementalBackup:
-						manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, "auto", tc.expectError)
+						manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, "auto", false /* expectEmpty */, tc.expectError)
 						if tc.expectError != "" {
 							return
 						}
+						require.NotNil(t, manifest)
 						defer func() {
 							lastBackupPos = manifest.Position
 						}()

go/vt/mysqlctl/backup.go

@@ -86,6 +86,8 @@
 	// backupCompressBlocks is the number of blocks that are processed
 	// once before the writer blocks
 	backupCompressBlocks = 2
+
+	EmptyBackupMessage = "no new data to backup, skipping it"
 )
 
 func init() {
@@ -168,14 +170,20 @@
 	}
 
 	// Take the backup, and either AbortBackup or EndBackup.
-	usable, err := be.ExecuteBackup(ctx, beParams, bh)
+	backupResult, err := be.ExecuteBackup(ctx, beParams, bh)
 	logger := params.Logger
 	var finishErr error
-	if usable {
-		finishErr = bh.EndBackup(ctx)
-	} else {
+	switch backupResult {
+	case BackupUnusable:
 		logger.Errorf2(err, "backup is not usable, aborting it")
 		finishErr = bh.AbortBackup(ctx)
+	case BackupEmpty:
+		logger.Infof(EmptyBackupMessage)
+		// While an empty backup is considered "successful", it should leave no trace.
+		// We therefore ensire to clean up an backup files/directories/entries.
+		finishErr = bh.AbortBackup(ctx)
+	case BackupUsable:
+		finishErr = bh.EndBackup(ctx)
 	}
 	if err != nil {
 		if finishErr != nil {

go/vt/mysqlctl/backup_blackbox_test.go

@@ -149,7 +149,7 @@ func TestExecuteBackup(t *testing.T) {
 
 	fakeStats := backupstats.NewFakeStats()
 
-	ok, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
+	backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
 		Logger: logutil.NewConsoleLogger(),
 		Mysqld: mysqld,
 		Cnf: &mysqlctl.Mycnf{
@@ -167,7 +167,7 @@ func TestExecuteBackup(t *testing.T) {
 	}, bh)
 
 	require.NoError(t, err)
-	assert.True(t, ok)
+	assert.Equal(t, mysqlctl.BackupUsable, backupResult)
 
 	var destinationCloseStats int
 	var destinationOpenStats int
@@ -209,7 +209,7 @@ func TestExecuteBackup(t *testing.T) {
 	mysqld.ExpectedExecuteSuperQueryCurrent = 0 // resest the index of what queries we've run
 	mysqld.ShutdownTime = time.Minute           // reminder that shutdownDeadline is 1s
 
-	ok, err = be.ExecuteBackup(ctx, mysqlctl.BackupParams{
+	backupResult, err = be.ExecuteBackup(ctx, mysqlctl.BackupParams{
 		Logger: logutil.NewConsoleLogger(),
 		Mysqld: mysqld,
 		Cnf: &mysqlctl.Mycnf{
@@ -225,7 +225,7 @@ func TestExecuteBackup(t *testing.T) {
 	}, bh)
 
 	assert.Error(t, err)
-	assert.False(t, ok)
+	assert.Equal(t, mysqlctl.BackupUnusable, backupResult)
 }
 
 func TestExecuteBackupWithSafeUpgrade(t *testing.T) {
@@ -296,7 +296,7 @@ func TestExecuteBackupWithSafeUpgrade(t *testing.T) {
 		"SET GLOBAL innodb_fast_shutdown=0": {},
 	}
 
-	ok, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
+	backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
 		Logger: logutil.NewConsoleLogger(),
 		Mysqld: mysqld,
 		Cnf: &mysqlctl.Mycnf{
@@ -314,7 +314,7 @@ func TestExecuteBackupWithSafeUpgrade(t *testing.T) {
 	}, bh)
 
 	require.NoError(t, err)
-	assert.True(t, ok)
+	assert.Equal(t, mysqlctl.BackupUsable, backupResult)
 }
 
 // TestExecuteBackupWithCanceledContext tests the ability of the backup function to gracefully handle cases where errors
@@ -383,7 +383,7 @@ func TestExecuteBackupWithCanceledContext(t *testing.T) {
 	cancelledCtx, cancelCtx := context.WithCancel(context.Background())
 	cancelCtx()
 
-	ok, err := be.ExecuteBackup(cancelledCtx, mysqlctl.BackupParams{
+	backupResult, err := be.ExecuteBackup(cancelledCtx, mysqlctl.BackupParams{
 		Logger: logutil.NewConsoleLogger(),
 		Mysqld: mysqld,
 		Cnf: &mysqlctl.Mycnf{
@@ -403,7 +403,7 @@ func TestExecuteBackupWithCanceledContext(t *testing.T) {
 	require.Error(t, err)
 	// all four files will fail
 	require.ErrorContains(t, err, "context canceled;context canceled;context canceled;context canceled")
-	assert.False(t, ok)
+	assert.Equal(t, mysqlctl.BackupUnusable, backupResult)
 }
 
 // TestExecuteRestoreWithCanceledContext tests the ability of the restore function to gracefully handle cases where errors
@@ -468,7 +468,7 @@ func TestExecuteRestoreWithTimedOutContext(t *testing.T) {
 	defer mysqld.Close()
 	mysqld.ExpectedExecuteSuperQueryList = []string{"STOP SLAVE", "START SLAVE"}
 
-	ok, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
+	backupResult, err := be.ExecuteBackup(ctx, mysqlctl.BackupParams{
 		Logger: logutil.NewConsoleLogger(),
 		Mysqld: mysqld,
 		Cnf: &mysqlctl.Mycnf{
@@ -486,7 +486,7 @@ func TestExecuteRestoreWithTimedOutContext(t *testing.T) {
 	}, bh)
 
 	require.NoError(t, err)
-	assert.True(t, ok)
+	assert.Equal(t, mysqlctl.BackupUsable, backupResult)
 
 	// Now try to restore the above backup.
 	bh = filebackupstorage.NewBackupHandle(nil, "", "", true)

go/vt/mysqlctl/backupengine.go

@@ -45,9 +45,17 @@ var (
 	backupEngineImplementation = builtinBackupEngineName
 )
 
+type BackupResult int
+
+const (
+	BackupUnusable BackupResult = iota
+	BackupEmpty
+	BackupUsable
+)
+
 // BackupEngine is the interface to take a backup with a given engine.
 type BackupEngine interface {
-	ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (bool, error)
+	ExecuteBackup(ctx context.Context, params BackupParams, bh backupstorage.BackupHandle) (BackupResult, error)
 	ShouldDrainForBackup(req *tabletmanagerdatapb.BackupRequest) bool
 }
 

go/vt/mysqlctl/binlogs_gtid.go

@@ -104,8 +104,7 @@ func ChooseBinlogsForIncrementalBackup(
 		// The other thing to validate, is that we can't allow a situation where the backup-GTIDs have entries not covered
 		// by our binary log's Previous-GTIDs (padded with purged GTIDs). Because that means we can't possibly restore to
 		// such position.
-		prevGTIDsUnionPurged := prevGTIDsUnion.Union(purgedGTIDSet)
-		if !prevGTIDsUnionPurged.Contains(backupFromGTIDSet) {
+		if prevGTIDsUnionPurged := prevGTIDsUnion.Union(purgedGTIDSet); !prevGTIDsUnionPurged.Contains(backupFromGTIDSet) {
 			return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION,
 				"Mismatching GTID entries. Requested backup pos has entries not found in the binary logs, and binary logs have entries not found in the requested backup pos. Neither fully contains the other.\n- Requested pos=%v\n- binlog pos=%v\n- purgedGTIDSet=%v\n- union=%v\n- union purged=%v",
 				backupFromGTIDSet, previousGTIDsPos.GTIDSet, purgedGTIDSet, prevGTIDsUnion, prevGTIDsUnionPurged)
@@ -133,7 +132,16 @@ func ChooseBinlogsForIncrementalBackup(
 		}
 		return binaryLogsToBackup, incrementalBackupFromGTID, incrementalBackupToGTID, nil
 	}
-	return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "no binary logs to backup (increment is empty)")
+	if prevGTIDsUnion.Union(purgedGTIDSet).Equal(backupFromGTIDSet) {
+		// This means we've iterated over all binary logs, and as it turns out, the backup pos is
+		// identical to the Previous-GTIDs of the last binary log. But, we also know that we ourselves
+		// have flushed the binary logs so as to generate the new (now last) binary log.
+		// Which means, from the Pos of the backup till the time we issued FLUSH BINARY LOGS, there
+		// were no new GTID entries. The database was performed no writes during that period,
+		// so we have no entries to backup and the backup is therefore empty.
+		return nil, "", "", nil
+	}
+	return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "cannot find binary logs that cover requested GTID range. backupFromGTIDSet=%v, prevGTIDsUnion=%v", backupFromGTIDSet.String(), prevGTIDsUnion.String())
 }
 
 // IsValidIncrementalBakcup determines whether the given manifest can be used to extend a backup