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 replication.Position, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
+func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incrementalFromPos replication.Position, expectEmpty bool, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
 	numBackups := len(waitForNumBackups(t, -1))
+
 	incrementalFromPosArg := "auto"
 	if !incrementalFromPos.IsZero() {
 		incrementalFromPosArg = replication.EncodePosition(incrementalFromPos)
@@ -1269,18 +1270,24 @@ func testReplicaIncrementalBackup(t *testing.T, replica *cluster.Vttablet, incre
 	}
 	require.NoErrorf(t, err, "output: %v", output)
 
+	if expectEmpty {
+		require.Contains(t, output, "backup is empty")
+		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 replication.Position, expectError string) (manifest *mysqlctl.BackupManifest, backupName string) {
+func TestReplicaIncrementalBackup(t *testing.T, replicaIndex int, incrementalFromPos replication.Position, 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

@@ -128,26 +128,33 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase)
 			writeBeforeBackup bool
 			fromFullPosition  bool
 			autoPosition      bool
+			expectEmpty       bool
 			expectError       string
 		}{
 			{
 				name: "first incremental backup",
 			},
 			{
-				name:        "fail1",
-				expectError: "no binary logs to backup",
+				name:        "empty1",
+				expectEmpty: true,
 			},
 			{
-				name:        "fail2",
-				expectError: "no binary logs to backup",
+				name:         "empty2",
+				autoPosition: true,
+				expectEmpty:  true,
+			},
+			{
+				name:        "empty3",
+				expectEmpty: true,
 			},
 			{
 				name:              "make writes, succeed",
 				writeBeforeBackup: true,
 			},
 			{
-				name:        "fail, no binary logs to backup",
-				expectError: "no binary logs to backup",
+				name:         "empty again",
+				autoPosition: true,
+				expectEmpty:  true,
 			},
 			{
 				name:              "make writes again, succeed",
@@ -159,9 +166,9 @@ func ExecTestIncrementalBackupAndRestoreToPos(t *testing.T, tcase *PITRTestCase)
 				autoPosition:      true,
 			},
 			{
-				name:         "fail auto position, no binary logs to backup",
+				name:         "empty again, based on auto position",
 				autoPosition: true,
-				expectError:  "no binary logs to backup",
+				expectEmpty:  true,
 			},
 			{
 				name:              "auto position, make writes again, succeed",
@@ -200,10 +207,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
 				}()
@@ -324,26 +336,33 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 			writeBeforeBackup bool
 			fromFullPosition  bool
 			autoPosition      bool
+			expectEmpty       bool
 			expectError       string
 		}{
 			{
 				name: "first incremental backup",
 			},
 			{
-				name:        "fail1",
-				expectError: "no binary logs to backup",
+				name:        "empty1",
+				expectEmpty: true,
 			},
 			{
-				name:        "fail2",
-				expectError: "no binary logs to backup",
+				name:         "empty2",
+				autoPosition: true,
+				expectEmpty:  true,
+			},
+			{
+				name:        "empty3",
+				expectEmpty: true,
 			},
 			{
 				name:              "make writes, succeed",
 				writeBeforeBackup: true,
 			},
 			{
-				name:        "fail, no binary logs to backup",
-				expectError: "no binary logs to backup",
+				name:         "empty again",
+				autoPosition: true,
+				expectEmpty:  true,
 			},
 			{
 				name:              "make writes again, succeed",
@@ -355,9 +374,9 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 				autoPosition:      true,
 			},
 			{
-				name:         "fail auto position, no binary logs to backup",
+				name:         "empty again, based on auto position",
 				autoPosition: true,
-				expectError:  "no binary logs to backup",
+				expectEmpty:  true,
 			},
 			{
 				name:              "auto position, make writes again, succeed",
@@ -393,11 +412,16 @@ func ExecTestIncrementalBackupAndRestoreToTimestamp(t *testing.T, tcase *PITRTes
 						incrementalFromPos = 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.
@@ -664,10 +688,11 @@ func ExecTestIncrementalBackupOnTwoTablets(t *testing.T, tcase *PITRTestCase) {
 						lastBackupPos = fullBackupPos
 					case operationIncrementalBackup:
 						var incrementalFromPos replication.Position // keep zero, we will use "auto"
-						manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, incrementalFromPos, tc.expectError)
+						manifest, _ := TestReplicaIncrementalBackup(t, tc.replicaIndex, incrementalFromPos, false /* expectEmpty */, tc.expectError)
 						if tc.expectError != "" {
 							return
 						}
+						require.NotNil(t, manifest)
 						defer func() {
 							lastBackupPos = manifest.Position
 						}()

go/vt/mysqlctl/backup.go

@@ -168,14 +168,18 @@
 	}
 
 	// 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("backup is empty")
+		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 completely silent during that period.
+		// We have nothing to backup. The backup is empty.
+		return nil, "", "", nil
+	}
+	return nil, "", "", vterrors.Errorf(vtrpc.Code_FAILED_PRECONDITION, "no binary logs to backup. backupFromGTIDSet=%v, prevGTIDsUnion=%v", backupFromGTIDSet.String(), prevGTIDsUnion.String())
 }
 
 // IsValidIncrementalBakcup determines whether the given manifest can be used to extend a backup

go/vt/mysqlctl/binlogs_gtid_test.go

@@ -85,7 +85,7 @@ func TestChooseBinlogsForIncrementalBackup(t *testing.T) {
 			name:          "last binlog excluded, no binlogs found",
 			previousGTIDs: basePreviousGTIDs,
 			backupPos:     "16b1039f-22b6-11ed-b765-0a43f95f28a3:1-331",
-			expectError:   "no binary logs to backup",
+			expectBinlogs: nil,
 		},
 		{
 			name:          "backup pos beyond all binlogs",
@@ -294,13 +294,14 @@ func TestChooseBinlogsForIncrementalBackup(t *testing.T) {
 				return
 			}
 			require.NoError(t, err)
-			require.NotEmpty(t, binlogsToBackup)
 			assert.Equal(t, tc.expectBinlogs, binlogsToBackup)
-			if tc.previousGTIDs[binlogsToBackup[0]] != "" {
-				assert.Equal(t, tc.previousGTIDs[binlogsToBackup[0]], fromGTID)
+			if len(binlogsToBackup) > 0 {
+				if tc.previousGTIDs[binlogsToBackup[0]] != "" {
+					assert.Equal(t, tc.previousGTIDs[binlogsToBackup[0]], fromGTID)
+				}
+				assert.Equal(t, tc.previousGTIDs[binlogs[len(binlogs)-1]], toGTID)
+				assert.NotEqual(t, fromGTID, toGTID)
 			}
-			assert.Equal(t, tc.previousGTIDs[binlogs[len(binlogs)-1]], toGTID)
-			assert.NotEqual(t, fromGTID, toGTID)
 		})
 	}
 }