backup: remove old incremental backup location lookup

In pre-22.2 backups, incremental backups were written to the same
directory as the full backup. This was removed in 22.2, but logic was
kept in place to maintain backwards compatibility and allow later
versions to restore from pre-22.2 backups.

However, given that we are now on 25.4 and are many major versions past
the removal of the old default location, the legacy logic is now being
removed to facilitate the development of backup indexes.

Epic: None

Release note: None

Author: kev-cao

pkg/backup/backup_test.go

@@ -9958,118 +9958,6 @@ func TestUserfileNormalizationIncrementalShowBackup(t *testing.T) {
 	sqlDB.Exec(t, query)
 }
 
-// TestBackupRestoreOldIncrementalDefaults tests that a call to restore
-// will correctly load an incremental backup in the old "default" directory.
-// That old default is literally just "the same directory as the full backup",
-// so write that manually with the `incremental_location` option then check that
-// a read without that option contains the same data.
-func TestBackupRestoreOldIncrementalDefault(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	skip.UnderRace(t, "multinode cluster setup times out under race, likely due to resource starvation.")
-
-	const numAccounts = 1
-	_, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, multiNode, numAccounts, InitManualReplication)
-	defer cleanupFn()
-
-	const c1, c2, c3 = `nodelocal://1/full/`, `nodelocal://1/full/`, `nodelocal://2/full/`
-
-	// Deliberately the same. We're simulating an incremental backup in the old
-	// default directory, i.e. the top-level collection directory.
-	const i1, i2, i3 = `nodelocal://1/full/`, `nodelocal://1/full/`, `nodelocal://2/full/`
-
-	collections := []string{
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", c1, url.QueryEscape("default")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", c2, url.QueryEscape("dc=dc1")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", c3, url.QueryEscape("dc=dc2")),
-	}
-
-	incrementals := []string{
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", i1, url.QueryEscape("default")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", i2, url.QueryEscape("dc=dc1")),
-		fmt.Sprintf("'%s?COCKROACH_LOCALITY=%s'", i3, url.QueryEscape("dc=dc2")),
-	}
-	tests := []struct {
-		dest []string
-		inc  []string
-	}{{dest: []string{collections[0]}, inc: []string{incrementals[0]}},
-		{dest: collections, inc: incrementals}}
-
-	for _, br := range tests {
-		dest := strings.Join(br.dest, ", ")
-		inc := strings.Join(br.inc, ", ")
-
-		if len(br.dest) > 1 {
-			dest = "(" + dest + ")"
-			inc = "(" + inc + ")"
-		}
-		// create db
-		sqlDB.Exec(t, `CREATE DATABASE fkdb`)
-		sqlDB.Exec(t, `CREATE TABLE fkdb.fk (ind INT)`)
-
-		for i := 0; i < 10; i++ {
-			sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, i)
-		}
-		fb := fmt.Sprintf("BACKUP DATABASE fkdb INTO %s", dest)
-		sqlDB.Exec(t, fb)
-
-		sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, 200)
-
-		sib := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location=%s", dest, inc)
-		sqlDB.Exec(t, sib)
-
-		sir := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'inc_fkdb'", dest)
-		sqlDB.Exec(t, sir)
-
-		sqlDB.CheckQueryResults(t, `SELECT * FROM inc_fkdb.fk`, sqlDB.QueryStr(t, `SELECT * FROM fkdb.fk`))
-
-		sqlDB.Exec(t, "DROP DATABASE fkdb")
-		sqlDB.Exec(t, "DROP DATABASE inc_fkdb;")
-	}
-}
-
-func TestBackupRestoreErrorsOnBothDefaultsPopulated(t *testing.T) {
-	defer leaktest.AfterTest(t)()
-	defer log.Scope(t).Close(t)
-
-	const numAccounts = 1
-	_, sqlDB, _, cleanupFn := backupRestoreTestSetup(t, singleNode, numAccounts, InitManualReplication)
-	defer cleanupFn()
-
-	base := `'nodelocal://1/full/'`
-	oldInc := base
-	newInc := `'nodelocal://1/full/incrementals'`
-
-	// create db
-	sqlDB.Exec(t, `CREATE DATABASE fkdb`)
-	sqlDB.Exec(t, `CREATE TABLE fkdb.fk (ind INT)`)
-
-	for i := 0; i < 10; i++ {
-		sqlDB.Exec(t, `INSERT INTO fkdb.fk (ind) VALUES ($1)`, i)
-	}
-	fb := fmt.Sprintf("BACKUP DATABASE fkdb INTO %s", base)
-	sqlDB.Exec(t, fb)
-
-	sibOld := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location=%s", base, oldInc)
-	sqlDB.Exec(t, sibOld)
-
-	sibNew := fmt.Sprintf("BACKUP DATABASE fkdb INTO LATEST IN %s WITH incremental_location=%s", base, newInc)
-	sqlDB.Exec(t, sibNew)
-
-	irDefault := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'trad_fkdb'", base)
-	sqlDB.ExpectErr(t, "pq: Incremental layers found in both old and new default locations. "+
-		"Please choose a location manually with the `incremental_location` parameter.", irDefault)
-
-	irOld := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'trad_fkdb_old_default', "+
-		"incremental_location=%s", base, base)
-	sqlDB.Exec(t, irOld)
-
-	irNew := fmt.Sprintf("RESTORE DATABASE fkdb FROM LATEST IN %s WITH new_db_name = 'trad_fkdb_new_default', "+
-		"incremental_location=%s", base, newInc)
-	sqlDB.Exec(t, irNew)
-}
-
 // TestBackupRestoreSeparateExplicitIsDefault tests that a backup/restore round
 // trip using the 'incremental_location' parameter restores the same db as a BR
 // round trip without the parameter, even when that location is in fact the default.

pkg/backup/backupdest/backup_destination_test.go

@@ -133,10 +133,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 				inc5Time := inc4Time.Add(time.Minute * 30)
 				inc6Time := inc5Time.Add(time.Minute * 30)
 				inc7Time := inc6Time.Add(time.Minute * 30)
-				full3Time := inc7Time.Add(time.Minute * 30)
-				inc8Time := full3Time.Add(time.Minute * 30)
-				inc9Time := inc8Time.Add(time.Minute * 30)
-				full4Time := inc9Time.Add(time.Minute * 30)
 
 				// firstBackupChain is maintained throughout the tests as the history of
 				// backups that were taken based on the initial full backup.
@@ -155,6 +151,7 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 				testCollectionBackup := func(t *testing.T, backupTime time.Time,
 					expectedDefault, expectedSuffix, expectedIncDir string, expectedPrevBackups []string,
 					appendToLatest bool, subdir string, incrementalTo []string) {
+					t.Helper()
 
 					endTime := hlc.Timestamp{WallTime: backupTime.UnixNano()}
 
@@ -335,63 +332,6 @@ func TestBackupRestoreResolveDestination(t *testing.T) {
 						true /* intoLatest */, expectedSubdir, customIncrementalTo)
 					writeManifest(t, expectedDefault, inc7Time)
 				}
-
-				// A new full backup: BACKUP INTO collection
-				var backup3Location string
-				{
-					expectedSuffix := "/2020/12/25-103000.00"
-					expectedIncDir := ""
-					expectedDefault := fmt.Sprintf("nodelocal://1/%s%s?AUTH=implicit", t.Name(), expectedSuffix)
-					backup3Location = expectedDefault
-
-					testCollectionBackup(t, full3Time,
-						expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
-						false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
-					writeManifest(t, expectedDefault, full3Time)
-					// We also wrote a new full backup, so let's update the latest.
-					writeLatest(t, collectionLoc, expectedSuffix)
-				}
-
-				// A remote incremental into the third full backup: BACKUP INTO LATEST
-				// IN collection, BUT with a trick. Write a (fake) incremental backup
-				// to the old directory, to be sure that subsequent incremental backups
-				// go there as well (and not the newer incrementals/ subdir.)
-				{
-					expectedSuffix := "/2020/12/25-103000.00"
-					expectedIncDir := "/20201225/110000.00-20201225-103000.00"
-
-					// Writes the (fake) incremental backup.
-					oldStyleDefault := fmt.Sprintf("nodelocal://1/%s%s%s?AUTH=implicit",
-						t.Name(),
-						expectedSuffix, expectedIncDir)
-					writeManifest(t, oldStyleDefault, inc8Time)
-
-					expectedSuffix = "/2020/12/25-103000.00"
-					expectedIncDir = "/20201225/113000.00-20201225-110000.00"
-					expectedSubdir := expectedSuffix
-					expectedDefault := fmt.Sprintf("nodelocal://1/%s%s%s?AUTH=implicit",
-						t.Name(),
-						expectedSuffix, expectedIncDir)
-
-					testCollectionBackup(t, inc9Time,
-						expectedDefault, expectedSuffix, expectedIncDir, []string{backup3Location, oldStyleDefault},
-						true /* intoLatest */, expectedSubdir, noIncrementalStorage)
-					writeManifest(t, expectedDefault, inc9Time)
-				}
-
-				// A new full backup: BACKUP INTO collection
-				{
-					expectedSuffix := "/2020/12/25-120000.00"
-					expectedIncDir := ""
-					expectedDefault := fmt.Sprintf("nodelocal://1/%s%s?AUTH=implicit", t.Name(), expectedSuffix)
-
-					testCollectionBackup(t, full4Time,
-						expectedDefault, expectedSuffix, expectedIncDir, []string(nil),
-						false /* intoLatest */, noExplicitSubDir, noIncrementalStorage)
-					writeManifest(t, expectedDefault, full4Time)
-					// We also wrote a new full backup, so let's update the latest.
-					writeLatest(t, collectionLoc, expectedSuffix)
-				}
 			})
 		})
 	}

pkg/backup/backupdest/incrementals.go

@@ -196,45 +196,16 @@ func ResolveIncrementalsBackupLocation(
 		return incPaths, nil
 	}
 
-	resolvedIncrementalsBackupLocationOld, err := backuputils.AppendPaths(fullBackupCollections, subdir)
-	if err != nil {
-		return nil, err
-	}
-
-	// We can have >1 full backup collection specified, but each will have an
-	// incremental layer iff all of them do. So it suffices to check only the
-	// first.
-	// Check we can read from this location, though we don't need the backups here.
-	prevOld, err := backupsFromLocation(ctx, user, execCfg, resolvedIncrementalsBackupLocationOld[0])
-	if err != nil {
-		return nil, err
-	}
-
 	resolvedIncrementalsBackupLocation, err := backuputils.AppendPaths(fullBackupCollections, backupbase.DefaultIncrementalsSubdir, subdir)
 	if err != nil {
 		return nil, err
 	}
 
-	prev, err := backupsFromLocation(ctx, user, execCfg, resolvedIncrementalsBackupLocation[0])
+	_, err = backupsFromLocation(ctx, user, execCfg, resolvedIncrementalsBackupLocation[0])
 	if err != nil {
 		return nil, err
 	}
 
-	// TODO(bardin): This algorithm divides "destination resolution" and "actual backup lookup" for historical reasons,
-	// but this doesn't quite make sense now that destination resolution depends on backup lookup.
-	// Try to figure out a clearer way to organize this.
-	if len(prevOld) > 0 && len(prev) > 0 {
-		return nil, errors.New(
-			"Incremental layers found in both old and new default locations. " +
-				"Please choose a location manually with the `incremental_location` parameter.")
-	}
-
-	// If we have backups in the old default location, continue to use the old location.
-	if len(prevOld) > 0 {
-		return resolvedIncrementalsBackupLocationOld, nil
-	}
-
-	// Otherwise, use the new location.
 	return resolvedIncrementalsBackupLocation, nil
 }