Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-22.1: release-22.2: only check for introduced spans for online tables in restore #89688

Merged
merged 1 commit into from
Oct 11, 2022
Merged

release-22.1: release-22.2: only check for introduced spans for online tables in restore #89688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions pkg/ccl/backupccl/targets.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ func fullClusterTargetsBackup(
// they are also restore targets. This specifically entails checking the
// following invariant on each table we seek to restore: if the nth backup
// contains an online table in its backupManifest.Descriptors field or
// backupManifest. DescriptorsChanges field but the n-1th does not contain it in
// backupManifest.DescriptorsChanges field but the n-1th does not contain it online in
// its Descriptors field, then that table must be covered by the nth backup's
// manifest.IntroducedSpans field. Note: this check assumes that
// mainBackupManifests are sorted by Endtime and this check only applies to
Expand Down Expand Up @@ -362,11 +362,11 @@ func checkMissingIntroducedSpans(
continue
}

// Gather the tables included in the previous backup.
prevTables := make(map[descpb.ID]struct{})
// Gather the _online_ tables included in the previous backup.
prevOnlineTables := make(map[descpb.ID]struct{})
for _, desc := range mainBackupManifests[i-1].Descriptors {
if table, _, _, _ := descpb.FromDescriptor(&desc); table != nil {
prevTables[table.GetID()] = struct{}{}
if table, _, _, _ := descpb.FromDescriptor(&desc); table != nil && table.Public() {
prevOnlineTables[table.GetID()] = struct{}{}
}
}

Expand All @@ -381,6 +381,8 @@ func checkMissingIntroducedSpans(
tablesIntroduced[descpb.ID(tableID)] = struct{}{}
}

// requiredIntroduction affirms that the given table in backup i was either
// introduced in backup i, or included online in backup i-1.
requiredIntroduction := func(table *descpb.TableDescriptor) error {
if _, required := requiredTables[table.GetID()]; !required {
// The table is not required for the restore, so there's no need to check coverage.
Expand All @@ -397,7 +399,7 @@ func checkMissingIntroducedSpans(
return nil
}

if _, inPrevBackup := prevTables[table.GetID()]; inPrevBackup {
if _, inPrevBackup := prevOnlineTables[table.GetID()]; inPrevBackup {
// The table was included in the previous backup, which implies the
// table was online at prevBackup.Endtime, and therefore does not need
// to be introduced.
Expand All @@ -418,20 +420,20 @@ that was running an IMPORT at the time of the previous incremental in this chain
})
}

// If the table in the current backup was not in prevBackup.Descriptors,
// then it needs to be introduced.
for _, desc := range mainBackupManifests[i].Descriptors {
if table, _, _, _ := descpb.FromDescriptor(&desc); table != nil {
// Check that all online tables at backup time were either introduced or
// in the previous backup.
if table, _, _, _ := descpb.FromDescriptor(&desc); table != nil && table.Public() {
if err := requiredIntroduction(table); err != nil {
return err
}
}
}

// As described in the docstring for getReintroducedSpans(), there are cases
// where a descriptor may appear in manifest. DescriptorChanges but not
// manifest.Descriptors. If a descriptor is online at any moment during the
// backup interval, it needs to be reintroduced.
// where a descriptor may appear in manifest.DescriptorChanges but not
// manifest.Descriptors. If a descriptor switched from offline to online at
// any moment during the backup interval, it needs to be reintroduced.
for _, desc := range mainBackupManifests[i].DescriptorChanges {
if table, _, _, _ := descpb.FromDescriptor(desc.Desc); table != nil && table.Public() {
if err := requiredIntroduction(table); err != nil {
Expand Down
16 changes: 16 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/in-progress-import-missing-data
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,6 +169,22 @@ SELECT * FROM d.silly_view;
----
0

# Start and pause another import to ensure the restore checker doesn't spuriously fail on
# descriptors with an in-progress import.
exec-sql
SET CLUSTER SETTING jobs.debug.pausepoints = 'import.after_ingest';
----

exec-sql
CREATE TABLE foo_offline (i INT PRIMARY KEY, s STRING);
----

import expect-pausepoint tag=b
IMPORT INTO foo_offline (i,s) CSV DATA ('nodelocal://0/export1/export*-n*.0.csv')
----
job paused at pausepoint


# Because BackupRestoreTestingKnobs.SkipDescriptorChangeIntroduction is turned on,
# this backup will be unable to introduce foo, foofoo, and goodfoo.
exec-sql
Expand Down