-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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.
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
backupccl: target offline importing tables if flag is set #85979
Conversation
d9ff717
to
ef2df29
Compare
RFAL! only the latest commit is relevant, the current test failures are unrelated (#85350 (comment)) |
ef2df29
to
431982b
Compare
pkg/ccl/backupccl/testdata/backup-restore/backup-dropped-descriptors-declarative
Outdated
Show resolved
Hide resolved
acc0df1
to
4560fb2
Compare
Historically, offline tables were excluded from backups (e.g. restoring or importing tables) because incremental backups could not reason correctly about their MVCC history. Now that all within tenant operations are now MVCC, BACKUP can begin incrementally backing up offline tables. Independent of the MVCC history thread, cockroachdb#83915 allowed backup to implicitly target _all_ offline descriptors (e.g. database, schema, table, type) to account for a class of compound online schema changes. This is a regression from the expected backup behavior and will get reverted in future work. This patch ensures that offline tables with importing data are not targetted iff an external flag is set. This patch merely refactors the internal targeting logic in backupresolver.ResolveTargetsToDescriptors, and does not change how backups behave. In other words, after this patch, no offline descriptors will get targeted, as all callers set the function's backupImports flag to false. A future PR will change these callsites. Informs cockroachdb#76722 Release note: none Release note (<category, see below>): <what> <show> <why>
4560fb2
to
f32c8fc
Compare
This new diff will only target in-progress imports iff an external flag is set. It doesn't change the behavior of any other kinds of offline descriptors; therefore, this diff is quite low impact. I realized that I needed to modify targets.go in order to RESTORE TABLE with an in-flight import correctly. @ajwerner We can delay this PR until after you refactor the schema changer to not depend on the offline state. Ideally, I can slip this in, so I can avoid doing duplicate implementation work. |
Closing for now. We can figure out how to filter offline descriptors in restore properly without modify DescMatchingTargets. |
Historically, offline tables were excluded from backups (e.g. restoring or
importing tables) because incremental backups could not reason correctly about
their MVCC history. Now that all within tenant operations are now MVCC, BACKUP
can begin incrementally backing up offline tables.
Independent of the MVCC history thread, #83915 allowed backup to implicitly
target all offline descriptors (e.g. database, schema, table, type) to
account for a class of compound online schema changes. This is a regression
from the expected backup behavior and will get reverted in future work.
This patch ensures that offline tables with importing data are not targetted
iff an external flag is set.
This patch merely refactors the internal targeting logic in
backupresolver.ResolveTargetsToDescriptors, and does not change how backups
behave. In other words, after this patch, no offline descriptors will get
targeted, as all callers set the function's backupImports flag to false. A
future PR will change these callsites.
Informs #76722
Release note: none
Release justification: low risk change to a codepath that does not currently get called in production