Merge #148098

148098: restore: gracefully handle lookup of dropped temp system tables r=msbutler a=kev-cao

Previously, if a lookup was attempted on a dropped system table (e.g. during cleanup after a failed restore), an error would be surfaced. This caused the restore jobs in the reverting state to be stuck in a retry loop. However, if we are attempting to do cleanup, we can simply skip any dropped descriptors, which this commit teaches restore to do.

Epic: CRDB-51394

Fixes: #148088

Release note: Restore no longer gets stuck in the reverting state after failed cleanup of dropped temporary system tables.

Co-authored-by: Kevin Cao <[email protected]>

Author: craig[bot]

pkg/backup/restore_job.go

@@ -55,6 +55,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/typedesc"
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scbackup"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
@@ -2080,6 +2082,12 @@ func (r *restoreResumer) doResume(ctx context.Context, execCtx interface{}) erro
 		}
 	}
 
+	if err := p.ExecCfg().JobRegistry.CheckPausepoint(
+		"restore.after_cleanup_temp_system_tables",
+	); err != nil {
+		return err
+	}
+
 	if details.DescriptorCoverage != tree.RequestedDescriptors {
 		// Bump the version of the role membership table so that the cache is
 		// invalidated.
@@ -2766,24 +2774,20 @@ func (r *restoreResumer) dropDescriptors(
 	b := txn.KV().NewBatch()
 	const kvTrace = false
 	// Collect the tables into mutable versions.
-	mutableTables := make([]*tabledesc.Mutable, len(details.TableDescs))
-	for i := range details.TableDescs {
-		var err error
-		mutableTables[i], err = descsCol.MutableByID(txn.KV()).Table(ctx, details.TableDescs[i].ID)
-		if err != nil {
-			return err
-		}
-		// Ensure that the version matches what we expect. In the case that it
-		// doesn't, it's not really clear what to do. Just log and carry on. If the
-		// descriptors have already been published, then there's nothing to fuss
-		// about so we only do this check if they have not been published.
-		if !details.DescriptorsPublished {
-			if got, exp := mutableTables[i].Version, details.TableDescs[i].Version; got != exp {
-				log.Errorf(ctx, "version changed for restored descriptor %d before "+
-					"drop: got %d, expected %d", mutableTables[i].GetID(), got, exp)
-			}
+	mutableTables, err := getUndroppedTablesFromRestore(
+		ctx, txn.KV(), details, descsCol,
+	)
+	if err != nil {
+		return errors.Wrap(err, "getting mutable tables from restore")
+	}
+	// Ensure that the table versions matches what we expect. In the case that it
+	// doesn't, it's not really clear what to do. Just log and carry on. If the
+	// descriptors have already been published, then there's nothing to fuss
+	// about so we only do this check if they have not been published.
+	if !details.DescriptorsPublished {
+		if err := checkRestoredTableDescriptorVersions(details, mutableTables); err != nil {
+			log.Errorf(ctx, "table version mismatch during drop: %v", err)
 		}
-
 	}
 
 	// Remove any back references installed from existing types to tables being restored.
@@ -3363,6 +3367,58 @@ func (r *restoreResumer) cleanupTempSystemTables(ctx context.Context) error {
 	return nil
 }
 
+// getUndroppedTablesFromRestore retrieves all table descriptors, offline or
+// online, as listed in the restore details, excluding any tables that have been
+// dropped or marked as dropped. This helps avoid situations where the temporary
+// system database tables have already been copied over to the real system
+// database and dropped but attempting to load those temporary tables again
+// would result in an error.
+func getUndroppedTablesFromRestore(
+	ctx context.Context, txn *kv.Txn, details jobspb.RestoreDetails, descCol *descs.Collection,
+) ([]*tabledesc.Mutable, error) {
+	var tables []*tabledesc.Mutable
+	for _, desc := range details.TableDescs {
+		mutableTable, err := descCol.MutableByID(txn).Table(ctx, desc.ID)
+		if err != nil {
+			if pgerror.GetPGCode(err) == pgcode.UndefinedTable {
+				continue
+			}
+			return nil, err
+		}
+		if mutableTable.Dropped() {
+			continue
+		}
+		tables = append(tables, mutableTable)
+	}
+	return tables, nil
+}
+
+// checkeRestoredTableDescriptorVersions compares the versions of descriptors at
+// the time of restore with the versions of the restored tables. It returns an
+// error if any of the restored tables have a version that does not match the
+// version that was recorded at the time of restore in the job details.
+func checkRestoredTableDescriptorVersions(
+	details jobspb.RestoreDetails, restoredTables []*tabledesc.Mutable,
+) error {
+	versionsAtRestoreTime := make(map[descpb.ID]descpb.DescriptorVersion)
+	for _, desc := range details.TableDescs {
+		versionsAtRestoreTime[desc.ID] = desc.Version
+	}
+	for _, table := range restoredTables {
+		if expVersion, ok := versionsAtRestoreTime[table.GetID()]; ok {
+			if table.Version != expVersion {
+				return errors.Errorf(
+					"version mismatch for restored descriptor %d, expected version %d, got %d",
+					table.GetID(), expVersion, table.Version,
+				)
+			}
+		} else {
+			return errors.Errorf("restored table %d not found in restore details", table.GetID())
+		}
+	}
+	return nil
+}
+
 var _ jobs.Resumer = &restoreResumer{}
 
 func init() {

pkg/backup/restore_online.go

@@ -892,11 +892,11 @@ func setDescriptorsOffline(
 		return nil
 	}
 
-	for i := range details.TableDescs {
-		mutableTable, err := descCol.MutableByID(txn.KV()).Table(ctx, details.TableDescs[i].ID)
-		if err != nil {
-			return err
-		}
+	mutableTables, err := getUndroppedTablesFromRestore(ctx, txn.KV(), details, descCol)
+	if err != nil {
+		return errors.Wrapf(err, "set descriptors offline: getting undropped tables from restore")
+	}
+	for _, mutableTable := range mutableTables {
 		if err := writeDesc(mutableTable); err != nil {
 			return err
 		}

pkg/backup/restore_online_test.go

@@ -204,12 +204,73 @@ func TestOnlineRestoreRecovery(t *testing.T) {
 	})
 }
 
+// We run full cluster online restore recovery in a separate environment since
+// it requires dropping all databases and will impact other tests.
+func TestFullClusterOnlineRestoreRecovery(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	tmpDir := t.TempDir()
+
+	defer nodelocal.ReplaceNodeLocalForTesting(tmpDir)()
+
+	const numAccounts = 1000
+
+	externalStorage := "nodelocal://1/backup"
+
+	_, sqlDB, _, cleanupFn := backupRestoreTestSetupWithParams(t, singleNode, numAccounts, InitManualReplication, orParams)
+	defer cleanupFn()
+
+	sqlDB.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.before_download'")
+	sqlDB.Exec(t, fmt.Sprintf("BACKUP INTO '%s'", externalStorage))
+
+	// Reset cluster for full cluster restore.
+	dbs := sqlDB.QueryStr(
+		t, "SELECT database_name FROM [SHOW DATABASES] WHERE database_name != 'system'",
+	)
+	sqlDB.Exec(t, "USE system")
+	for _, db := range dbs {
+		sqlDB.Exec(t, fmt.Sprintf("DROP DATABASE %s CASCADE", db[0]))
+	}
+
+	var linkJobID jobspb.JobID
+	sqlDB.QueryRow(
+		t,
+		fmt.Sprintf(
+			"RESTORE FROM LATEST IN '%s' WITH EXPERIMENTAL DEFERRED COPY, detached", externalStorage,
+		),
+	).Scan(&linkJobID)
+	jobutils.WaitForJobToSucceed(t, sqlDB, linkJobID)
+	var downloadJobID jobspb.JobID
+	sqlDB.QueryRow(t, latestDownloadJobIDQuery).Scan(&downloadJobID)
+	jobutils.WaitForJobToPause(t, sqlDB, downloadJobID)
+	corruptBackup(t, sqlDB, tmpDir, externalStorage)
+	sqlDB.Exec(t, "SET CLUSTER SETTING jobs.debug.pausepoints = ''")
+	sqlDB.Exec(t, fmt.Sprintf("RESUME JOB %d", downloadJobID))
+	jobutils.WaitForJobToFail(t, sqlDB, downloadJobID)
+}
+
 func corruptBackup(t *testing.T, sqlDB *sqlutils.SQLRunner, ioDir string, uri string) {
-	var filePath string
-	filePathQuery := fmt.Sprintf("SELECT path FROM [SHOW BACKUP FILES FROM LATEST IN '%s'] LIMIT 1", uri)
-	parsedURI, err := url.Parse(strings.Replace(uri, "'", "", -1))
-	sqlDB.QueryRow(t, filePathQuery).Scan(&filePath)
+	var filePath, spanStart, spanEnd string
+	// We delete the last SST file in SHOW BACKUP to ensure the deletion of an SST
+	// file that backs a user-table.
+	// https://github.com/cockroachdb/cockroach/issues/148408 illustrates how
+	// deleting the backing SST file of a system table will not necessarily cause
+	// a download job to fail.
+	filePathQuery := fmt.Sprintf(
+		`SELECT path, start_pretty, end_pretty FROM
+		(
+			SELECT row_number() OVER (), *
+			FROM [SHOW BACKUP FILES FROM LATEST IN '%s']
+		)
+		ORDER BY row_number DESC
+		LIMIT 1`,
+		uri,
+	)
+	parsedURI, err := url.Parse(strings.ReplaceAll(uri, "'", ""))
+	sqlDB.QueryRow(t, filePathQuery).Scan(&filePath, &spanStart, &spanEnd)
 	fullPath := filepath.Join(ioDir, parsedURI.Path, filePath)
+	t.Logf("deleting backup file %s covering span [%s, %s)", fullPath, spanStart, spanEnd)
 	require.NoError(t, err)
 	require.NoError(t, os.Remove(fullPath))
 }

pkg/backup/restore_test.go

@@ -11,6 +11,8 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/backup/backuptestutils"
+	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -58,3 +60,28 @@ func TestRestoreWithOpenTransaction(t *testing.T) {
 
 	userConn.Exec(t, "COMMIT")
 }
+
+// This test verifies that restore cleanup does not fail due to dropped
+// temporary system tables as described in #148088.
+func TestFailAfterCleanupSystemTables(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	clusterSize := 1
+	_, sqlDB, _, cleanupFn := backuptestutils.StartBackupRestoreTestCluster(t, clusterSize)
+	defer cleanupFn()
+
+	// Must set cluster setting before backup to ensure the setting is preserved.
+	sqlDB.Exec(
+		t, "SET CLUSTER SETTING jobs.debug.pausepoints = 'restore.after_cleanup_temp_system_tables'",
+	)
+	sqlDB.Exec(t, "BACKUP INTO 'nodelocal://1/backup'")
+
+	var jobID jobspb.JobID
+	sqlDB.QueryRow(t, "RESTORE FROM LATEST IN 'nodelocal://1/backup' WITH detached").Scan(&jobID)
+	sqlDB.Exec(t, "USE system")
+	jobutils.WaitForJobToPause(t, sqlDB, jobID)
+
+	sqlDB.Exec(t, "CANCEL JOB $1", jobID)
+	jobutils.WaitForJobToCancel(t, sqlDB, jobID)
+}