backupccl: defaultdb is not restored as MR in cluster restore
#74607
Conversation
|
This change is |
|
I don't love it, but I've tried to fix it in the most explicit way since it's to a prior release branch and I didn't want to introduce any subtle bugs. |
pkg/ccl/backupccl/restore_job.go
Outdated
| if typ.GetParentID() == defaultDBID && typ.GetKind() == descpb.TypeDescriptor_MULTIREGION_ENUM { | ||
| backedUpDefaultDBRegionConfig.RegionEnumID = typ.GetID() | ||
| break | ||
| } |
There was a problem hiding this comment.
Is it worth an assertion failure for the (believed impossible?) case of not finding the multi region enum?
There was a problem hiding this comment.
fortunately, we validate that the region enum ID we are pointing to actually refers to a type in the database - https://github.com/adityamaru/cockroach/blob/4a3ad27613d88c02ddfe8d4726c25b5d8411186f/pkg/sql/catalog/dbdesc/database_desc.go#L260-L260. So if this is set to something non-sensical it will error out when we go to write the database descriptor below.
Your comment got me thinking though that we don't really need this mapping since in a cluster restore, a type will have the same ID as when it was backed up. Removed the for loop entirely.
pkg/ccl/backupccl/restore_job.go
Outdated
| db := desc.(*dbdesc.Mutable) | ||
| db.SetRegionConfig(nil) |
There was a problem hiding this comment.
Do we need to do anything else to write this out or does DescsTxn somehow handle that for us? Typically, I see us creating a batch (b := txn.NewBatch()), writing the new descriptor to the batch with descCol.WriteToBatch and then running that batch (txn.Run(ctx, b)).
There was a problem hiding this comment.
ahhh good catch, fixing this.
adityamaru
force-pushed
the
mr-defaultdb-fix
branch
from
4a3ad27
to
1baaff9
Compare
This change fixes a bug where a backed up `defaultdb` with a MR LocalityConfig, is not restored as an MR database on cluster restore. In a cluster restore, we do not restore the database `defaultdb` from the BACKUP, but instead re-parent all backed up schema objects that were in `defaultdb` to point to the restoring cluster's `defaultdb`. This special handling is to account for the fact that every new cluster has a `defaultdb` at a static ID created on startup. As a result of this, we must set the RegionConfig of `defaultdb` on the restoring cluster, to match the config set on the backed up `defaultdb` descriptor. Fixes: cockroachdb#74586 Release note (bug fix): Fixes a bug where a backed up `defaultdb` that is configured to be MR, is not restored as a MR database on cluster restore. Release justification: bug fix.
adityamaru
force-pushed
the
mr-defaultdb-fix
branch
from
1baaff9
to
25d4398
Compare
good idea, done. |
This is a forward port of the test from cockroachdb#74607. In 22.1 onwards defalutdb is treated like any other database in the backup and so it should be restored with the correct RegionConfig without any intervention. Release note: None
This change fixes a bug where a backed up
defaultdbwith aMR LocalityConfig, is not restored as an MR database on cluster
restore.
In a cluster restore, we do not restore the database
defaultdbfromthe BACKUP, but instead re-parent all backed up schema objects that
were in
defaultdbto point to the restoring cluster'sdefaultdb.This special handling is to account for the fact that every new cluster
has a
defaultdbat a static ID created on startup.As a result of this, we must set the RegionConfig of
defaultdbonthe restoring cluster, to match the config set on the backed up
defaultdbdescriptor.Fixes: #74586
Release note (bug fix): Fixes a bug where a backed up
defaultdbthat is configured to be MR, is not restored as a MR database on
cluster restore.
Release justification: bug fix.