-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: add full cluster restore #43828
Conversation
f4efb54
to
a3a4c1f
Compare
deef2ff
to
55b26da
Compare
37ebabc
to
3c58318
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 8 of 10 files at r1, 6 of 8 files at r3, 5 of 6 files at r4.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @dt and @pbardea)
pkg/ccl/backupccl/backup.go, line 97 at r1 (raw file):
// Databases which are present in the cluster by default. var defaultUserDBs = map[string]struct{}{sessiondata.DefaultDatabaseName: {}, sessiondata.PgDatabaseName: {}}
this seems like it belongs in restore.go ?
pkg/ccl/backupccl/restore.go, line 407 at r2 (raw file):
numberOfIncrements := maxDescIDInBackup - uint32(maxDefaultID) for i := uint32(0); i <= numberOfIncrements; i++ { _, err = sql.GenerateUniqueDescID(ctx, p.ExecCfg().DB)
Huh, why the loop? why can't we just set it to maxDescIDInBackup?
pkg/ccl/backupccl/restore.go, line 431 at r2 (raw file):
} if found && descriptorCoverage == tree.AllDescriptors { if foundID > 52 {
is there a name for this constant we could use?
pkg/ccl/backupccl/restore.go, line 1707 at r4 (raw file):
if t := desc.Table(hlc.Timestamp{}); t != nil { // Don't count the default tables or system tables. if t.ParentID != sqlbase.SystemDB.ID {
i'd love to move this logic into a IsDefaultCreatedTable
or something method in sqlbase so that new tables get included correctly.
50b4b76
to
12c0512
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt)
pkg/ccl/backupccl/backup.go, line 97 at r1 (raw file):
Previously, dt (David Taylor) wrote…
this seems like it belongs in restore.go ?
Moved it over to sql/descriptor.go
so that it can be used to identify user created tables in general.
pkg/ccl/backupccl/restore.go, line 407 at r2 (raw file):
Previously, dt (David Taylor) wrote…
Huh, why the loop? why can't we just set it to maxDescIDInBackup?
It looks like the key it writes to only expects increments. Specifically when this key is incremented the comments say: "If the key exists but was set using Put or CPut an error will be returned."
pkg/ccl/backupccl/restore.go, line 431 at r2 (raw file):
Previously, dt (David Taylor) wrote…
is there a name for this constant we could use?
Yep, moved this to the sql
package along with helpers to identify user created tables.
pkg/ccl/backupccl/restore.go, line 1707 at r4 (raw file):
Previously, dt (David Taylor) wrote…
i'd love to move this logic into a
IsDefaultCreatedTable
or something method in sqlbase so that new tables get included correctly.
Moved this over to sql/descriptor.go
along with the list of expected user tables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt and @pbardea)
pkg/ccl/backupccl/restore.go, line 407 at r2 (raw file):
Previously, pbardea (Paul Bardea) wrote…
It looks like the key it writes to only expects increments. Specifically when this key is incremented the comments say: "If the key exists but was set using Put or CPut an error will be returned."
huh, interesting. let's put a TODO and follow up with KV to see if we can change this later.
12c0512
to
49a247a
Compare
This commit adds the ability to entirely restore a full cluster backup. This includes replacing the contents of some system tables to match the data in the backup. Full cluster restore can be used using the `RESTORE FROM ...` syntax and is expected to be run on a new cluster with no user data. Release note (enterprise change): Add full cluster restore feature which restores all the information contained in a full cluster backup. This includes all of the user data as well as relevant data from system tables. It is expected to be run on a new cluster with no user data.
49a247a
to
e168928
Compare
bors r+ |
43828: backupccl: add full cluster restore r=pbardea a=pbardea This commit adds the ability to entirely restore a full cluster backup. This includes replacing the contents of some system tables to match the data in the backup. Full cluster restore can be used using the `RESTORE FROM ...` syntax and is expected to be run on a new cluster with no user data. Release note (enterprise change): Add full cluster restore feature which restores all the information contained in a full cluster backup. This includes all of the user data as well as relevant data from system tables. It is expected to be run on a new cluster with no user data. 44684: sql: move the existing savepoint logic to a separate file r=yuzefovich a=knz This commit re-organizes the code without any functional change. (Split from #43051 for ease of review.) Co-authored-by: Paul Bardea <pbardea@gmail.com> Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Build succeeded |
This commit adds the ability to entirely restore a full cluster backup.
This includes replacing the contents of some system tables to match the
data in the backup. Full cluster restore can be used using the
RESTORE FROM ...
syntax and is expected to be run on a new cluster with no userdata.
Release note (enterprise change): Add full cluster restore feature which
restores all the information contained in a full cluster backup. This
includes all of the user data as well as relevant data from system
tables. It is expected to be run on a new cluster with no user data.