Skip to content

Commit

Permalink
catalog: add maybeAddConstraintIDs to RestoreChanges
Browse files Browse the repository at this point in the history
All backups taken in versions 22.1 and later should already have
constraint IDs set. Technically, now that we are in v24.1, we no longer
support restoring from versions older than 22.1, but we still have
tests that restore from old versions. Until those fixtures are updated,
we need to handle the case where constraint IDs are not set already when
restoring a backup.

To prove that this works, I've resurrected a different test that
restores from a v21.1 backup, which was earlier deleted in 8b56efd.

Release note: None
  • Loading branch information
rafiss committed Mar 8, 2024
1 parent d2042ef commit eb64a92
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 0 deletions.
9 changes: 9 additions & 0 deletions pkg/ccl/backupccl/testdata/backup-restore/show_backup
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,15 @@
new-cluster name=s1 allow-implicit-access localities=eu-central-1,eu-north-1,us-east-1
----

link-backup cluster=s1 src-path=show_backup_validate,invalidDependOnBy_21.1 dest-path=invalidDependOnBy_21.1
----

# This backup intentionally has a dangling invalid depend on by reference.
query-sql regex=invalid\sdepended-on-by
SELECT * FROM [SHOW BACKUP VALIDATE FROM 'invalidDependOnBy_21.1' IN 'nodelocal://1/'];
----
true

link-backup cluster=s1 src-path=show_backup_validate,valid-22.2 dest-path=valid-22.2
----

Expand Down
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
'���
4 changes: 4 additions & 0 deletions pkg/sql/catalog/post_deserialization_changes.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,10 @@ const (
// references.
RemovedDuplicateIDsInRefs

// AddedConstraintIDs indicates that table descriptors had constraint ID
// added.
AddedConstraintIDs

// RemovedSelfEntryInSchemas corresponds to a change which occurred in
// database descriptors to recover from an earlier bug whereby when
// dropping a schema, we'd mark the database itself as though it was the
Expand Down
106 changes: 106 additions & 0 deletions pkg/sql/catalog/tabledesc/table_desc_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,16 @@ func (tdb *tableDescriptorBuilder) RunRestoreChanges(
tdb.changes.Add(catalog.UpgradedSequenceReference)
}

// All backups taken in versions 22.1 and later should already have
// constraint IDs set. Technically, now that we are in v24.1, we no longer
// support restoring from versions older than 22.1, but we still have
// tests that restore from old versions. Until those fixtures are updated,
// we need to handle the case where constraint IDs are not set already.
// TODO(rafi): remove this once all fixtures are updated. See: https://github.com/cockroachdb/cockroach/issues/120146.
if changed := maybeAddConstraintIDs(tdb.maybeModified); changed {
tdb.changes.Add(catalog.AddedConstraintIDs)
}

// Upgrade the declarative schema changer state
if scpb.MigrateDescriptorState(version, tdb.maybeModified.ParentID, tdb.maybeModified.DeclarativeSchemaChangerState) {
tdb.changes.Add(catalog.UpgradedDeclarativeSchemaChangerState)
Expand Down Expand Up @@ -778,6 +788,102 @@ func maybeFixPrimaryIndexEncoding(idx *descpb.IndexDescriptor) (hasChanged bool)
return true
}

// maybeAddConstraintIDs ensures that all constraints have an ID associated with
// them.
func maybeAddConstraintIDs(desc *descpb.TableDescriptor) (hasChanged bool) {
// Only assign constraint IDs to physical tables.
if !desc.IsTable() {
return false
}
// Collect pointers to constraint ID variables.
var idPtrs []*descpb.ConstraintID
if len(desc.PrimaryIndex.KeyColumnIDs) > 0 {
idPtrs = append(idPtrs, &desc.PrimaryIndex.ConstraintID)
}
for i := range desc.Indexes {
idx := &desc.Indexes[i]
if !idx.Unique || idx.UseDeletePreservingEncoding {
continue
}
idPtrs = append(idPtrs, &idx.ConstraintID)
}
checkByName := make(map[string]*descpb.TableDescriptor_CheckConstraint)
for i := range desc.Checks {
ck := desc.Checks[i]
idPtrs = append(idPtrs, &ck.ConstraintID)
checkByName[ck.Name] = ck
}
fkByName := make(map[string]*descpb.ForeignKeyConstraint)
for i := range desc.OutboundFKs {
fk := &desc.OutboundFKs[i]
idPtrs = append(idPtrs, &fk.ConstraintID)
fkByName[fk.Name] = fk
}
for i := range desc.InboundFKs {
idPtrs = append(idPtrs, &desc.InboundFKs[i].ConstraintID)
}
uwoiByName := make(map[string]*descpb.UniqueWithoutIndexConstraint)
for i := range desc.UniqueWithoutIndexConstraints {
uwoi := &desc.UniqueWithoutIndexConstraints[i]
idPtrs = append(idPtrs, &uwoi.ConstraintID)
uwoiByName[uwoi.Name] = uwoi
}
for _, m := range desc.GetMutations() {
if idx := m.GetIndex(); idx != nil && idx.Unique && !idx.UseDeletePreservingEncoding {
idPtrs = append(idPtrs, &idx.ConstraintID)
} else if c := m.GetConstraint(); c != nil {
switch c.ConstraintType {
case descpb.ConstraintToUpdate_CHECK, descpb.ConstraintToUpdate_NOT_NULL:
idPtrs = append(idPtrs, &c.Check.ConstraintID)
case descpb.ConstraintToUpdate_FOREIGN_KEY:
idPtrs = append(idPtrs, &c.ForeignKey.ConstraintID)
case descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX:
idPtrs = append(idPtrs, &c.UniqueWithoutIndexConstraint.ConstraintID)
}
}
}
// Set constraint ID counter to sane initial value.
var maxID descpb.ConstraintID
for _, p := range idPtrs {
if id := *p; id > maxID {
maxID = id
}
}
if desc.NextConstraintID <= maxID {
desc.NextConstraintID = maxID + 1
hasChanged = true
}
// Update zero constraint IDs using counter.
for _, p := range idPtrs {
if *p != 0 {
continue
}
*p = desc.NextConstraintID
desc.NextConstraintID++
hasChanged = true
}
// Reconcile constraint IDs between enforced slice and mutation.
for _, m := range desc.GetMutations() {
if c := m.GetConstraint(); c != nil {
switch c.ConstraintType {
case descpb.ConstraintToUpdate_CHECK, descpb.ConstraintToUpdate_NOT_NULL:
if other, ok := checkByName[c.Check.Name]; ok {
c.Check.ConstraintID = other.ConstraintID
}
case descpb.ConstraintToUpdate_FOREIGN_KEY:
if other, ok := fkByName[c.ForeignKey.Name]; ok {
c.ForeignKey.ConstraintID = other.ConstraintID
}
case descpb.ConstraintToUpdate_UNIQUE_WITHOUT_INDEX:
if other, ok := uwoiByName[c.UniqueWithoutIndexConstraint.Name]; ok {
c.UniqueWithoutIndexConstraint.ConstraintID = other.ConstraintID
}
}
}
}
return hasChanged
}

// maybeRemoveDuplicateIDsInRefs ensures that IDs in references to other tables
// are not duplicated.
func maybeRemoveDuplicateIDsInRefs(d *descpb.TableDescriptor) (hasChanged bool) {
Expand Down

0 comments on commit eb64a92

Please sign in to comment.