Skip to content

Commit

Permalink
sql: allow for pre-19.2 foreign keys in table validation
Browse files Browse the repository at this point in the history 
We introduced a bug in v20.2 where we failed to upgrade referenced
descriptors' FK representations from pre-19.2 descriptors when
validating cross-references for tables, leading to validation failures
that would make the table unusable. This PR gets rid of the validation
errors by having `Validate()` account for pre-19.2-style foreign keys on
referenced table descriptors.

Release note (bug fix): Fixes a bug where tables and metadata were
unavailable due to spurious `missing fk back reference` validation
errors.
  • Loading branch information
@thoszhang
thoszhang committed Nov 25, 2020
1 parent 4aabe96 commit dd23c13
Show file tree
Hide file tree
Showing 2 changed files with 224 additions and 19 deletions.
127 changes: 123 additions & 4 deletions pkg/sql/catalog/tabledesc/structured.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1496,10 +1496,72 @@ func (desc *Immutable) validateCrossReferences(ctx context.Context, dg catalog.D
}
return nil
})
if !found {
return errors.AssertionFailedf("missing fk back reference %q to %q from %q",
if found {
continue
}
// In 20.2 we introduced a bug where we fail to upgrade the FK references
// on the referenced descriptors from their pre-19.2 format when reading
// them during validation (#57032). So we account for the possibility of
// un-upgraded foreign key references on the other table. This logic
// somewhat parallels the logic in maybeUpgradeForeignKeyRepOnIndex.
unupgradedFKsPresent := false
if err := referencedTable.ForeachIndex(catalog.IndexOpts{}, func(
referencedIdx *descpb.IndexDescriptor, isPrimary bool,
) error {
if found {
// TODO (lucy): If we ever revisit the tabledesc.Immutable methods, add
// a way to break out of the index loop.
return nil
}
if len(referencedIdx.ReferencedBy) > 0 {
unupgradedFKsPresent = true
} else {
return nil
}
// Determine whether the index on the other table is a unique index that
// could support this FK constraint.
if !referencedIdx.IsValidReferencedIndex(fk.ReferencedColumnIDs) {
return nil
}
// Now check the backreferences. Backreferences in ReferencedBy only had
// Index and Table populated.
for i := range referencedIdx.ReferencedBy {
backref := &referencedIdx.ReferencedBy[i]
if backref.Table != desc.ID {
continue
}
// Look up the index that the un-upgraded reference refers to and
// see if that index could support the foreign key reference. (Note
// that it shouldn't be possible for this index to not exist. See
// planner.MaybeUpgradeDependentOldForeignKeyVersionTables, which is
// called from the drop index implementation.)
originalOriginIndex, err := desc.FindIndexByID(backref.Index)
if err != nil {
return errors.AssertionFailedf(
"missing index %d on %q from pre-19.2 foreign key "+
"backreference %q on %q",
backref.Index, desc.Name, fk.Name, referencedTable.GetName(),
)
}
if originalOriginIndex.IsValidOriginIndex(fk.OriginColumnIDs) {
found = true
break
}
}
return nil
}); err != nil {
return err
}
if found {
continue
}
if unupgradedFKsPresent {
return errors.AssertionFailedf("missing fk back reference %q to %q "+
"from %q (un-upgraded foreign key references present)",
fk.Name, desc.Name, referencedTable.GetName())
}
return errors.AssertionFailedf("missing fk back reference %q to %q from %q",
fk.Name, desc.Name, referencedTable.GetName())
}
for i := range desc.InboundFKs {
backref := &desc.InboundFKs[i]
Expand All @@ -1515,10 +1577,67 @@ func (desc *Immutable) validateCrossReferences(ctx context.Context, dg catalog.D
}
return nil
})
if !found {
return errors.AssertionFailedf("missing fk forward reference %q to %q from %q",
if found {
continue
}
// In 20.2 we introduced a bug where we fail to upgrade the FK references
// on the referenced descriptors from their pre-19.2 format when reading
// them during validation (#57032). So we account for the possibility of
// un-upgraded foreign key references on the other table. This logic
// somewhat parallels the logic in maybeUpgradeForeignKeyRepOnIndex.
unupgradedFKsPresent := false
if err := originTable.ForeachIndex(catalog.IndexOpts{}, func(
originIdx *descpb.IndexDescriptor, isPrimary bool,
) error {
if found {
// TODO (lucy): If we ever revisit the tabledesc.Immutable methods, add
// a way to break out of the index loop.
return nil
}
fk := originIdx.ForeignKey
if fk.IsSet() {
unupgradedFKsPresent = true
} else {
return nil
}
// Determine whether the index on the other table is a index that could
// support this FK constraint on the referencing side. Such an index would
// have been required in earlier versions.
if !originIdx.IsValidOriginIndex(backref.OriginColumnIDs) {
return nil
}
if fk.Table != desc.ID {
return nil
}
// Look up the index that the un-upgraded reference refers to and
// see if that index could support the foreign key reference. (Note
// that it shouldn't be possible for this index to not exist. See
// planner.MaybeUpgradeDependentOldForeignKeyVersionTables, which is
// called from the drop index implementation.)
originalReferencedIndex, err := desc.FindIndexByID(fk.Index)
if err != nil {
return errors.AssertionFailedf(
"missing index %d on %q from pre-19.2 foreign key forward reference %q on %q",
fk.Index, desc.Name, backref.Name, originTable.GetName(),
)
}
if originalReferencedIndex.IsValidReferencedIndex(backref.OriginColumnIDs) {
found = true
}
return nil
}); err != nil {
return err
}
if found {
continue
}
if unupgradedFKsPresent {
return errors.AssertionFailedf("missing fk forward reference %q to %q from %q "+
"(un-upgraded foreign key references present)",
backref.Name, desc.Name, originTable.GetName())
}
return errors.AssertionFailedf("missing fk forward reference %q to %q from %q",
backref.Name, desc.Name, originTable.GetName())
}

for _, index := range desc.AllNonDropIndexes() {
Expand Down
116 changes: 101 additions & 15 deletions pkg/sql/catalog/tabledesc/structured_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -669,7 +669,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
otherDescs []descpb.TableDescriptor
}{
// Foreign keys
{
{ // 0
err: `invalid foreign key: missing table=52: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -689,7 +689,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
otherDescs: nil,
},
{
{ // 1
err: `missing fk back reference "fk" to "foo" from "baz"`,
desc: descpb.TableDescriptor{
ID: 51,
Expand All @@ -715,7 +715,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
FormatVersion: descpb.InterleavedFormatVersion,
}},
},
{
{ // 2
err: `invalid foreign key backreference: missing table=52: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -734,7 +734,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
},
},
{
{ // 3
err: `missing fk forward reference "fk" to "foo" from "baz"`,
desc: descpb.TableDescriptor{
ID: 51,
Expand Down Expand Up @@ -764,9 +764,93 @@ func TestValidateCrossTableReferences(t *testing.T) {
FormatVersion: descpb.InterleavedFormatVersion,
}},
},
{ // 4
// Regression test for #57066: We can handle one of the referenced tables
// having a pre-19.2 foreign key reference.
err: "",
desc: descpb.TableDescriptor{
ID: 51,
Name: "foo",
ParentID: 1,
UnexposedParentSchemaID: keys.PublicSchemaID,
FormatVersion: descpb.InterleavedFormatVersion,
Indexes: []descpb.IndexDescriptor{
{
ID: 2,
ColumnIDs: []descpb.ColumnID{1, 2},
},
},
OutboundFKs: []descpb.ForeignKeyConstraint{
{
Name: "fk",
ReferencedTableID: 52,
ReferencedColumnIDs: []descpb.ColumnID{1},
OriginTableID: 51,
OriginColumnIDs: []descpb.ColumnID{1},
},
},
},
otherDescs: []descpb.TableDescriptor{{
ID: 52,
Name: "baz",
ParentID: 1,
UnexposedParentSchemaID: keys.PublicSchemaID,
FormatVersion: descpb.InterleavedFormatVersion,
Indexes: []descpb.IndexDescriptor{
{
Unique: true,
ColumnIDs: []descpb.ColumnID{1},
ReferencedBy: []descpb.ForeignKeyReference{{Table: 51, Index: 2}},
},
},
}},
},
{ // 5
// Regression test for #57066: We can handle one of the referenced tables
// having a pre-19.2 foreign key reference.
err: "",
desc: descpb.TableDescriptor{
ID: 51,
Name: "foo",
ParentID: 1,
UnexposedParentSchemaID: keys.PublicSchemaID,
FormatVersion: descpb.InterleavedFormatVersion,
Indexes: []descpb.IndexDescriptor{
{
ID: 2,
ColumnIDs: []descpb.ColumnID{1},
Unique: true,
},
},
InboundFKs: []descpb.ForeignKeyConstraint{
{
Name: "fk",
ReferencedTableID: 51,
ReferencedColumnIDs: []descpb.ColumnID{1},
OriginTableID: 52,
OriginColumnIDs: []descpb.ColumnID{1},
},
},
},
otherDescs: []descpb.TableDescriptor{{
ID: 52,
Name: "baz",
ParentID: 1,
UnexposedParentSchemaID: keys.PublicSchemaID,
FormatVersion: descpb.InterleavedFormatVersion,
Indexes: []descpb.IndexDescriptor{
{
ID: 2,
Unique: true,
ColumnIDs: []descpb.ColumnID{1},
ForeignKey: descpb.ForeignKeyReference{Table: 51, Index: 2},
},
},
}},
},

// Interleaves
{
{ // 6
err: `invalid interleave: missing table=52 index=2: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -783,7 +867,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
otherDescs: nil,
},
{
{ // 7
err: `invalid interleave: missing table=baz index=2: index-id "2" does not exist`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -805,7 +889,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
UnexposedParentSchemaID: keys.PublicSchemaID,
}},
},
{
{ // 8
err: `missing interleave back reference to "foo"@"bar" from "baz"@"qux"`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -831,7 +915,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
}},
},
{
{ // 9
err: `invalid interleave backreference table=52 index=2: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -844,7 +928,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
},
},
{
{ // 10
err: `invalid interleave backreference table=baz index=2: index-id "2" does not exist`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -863,7 +947,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
UnexposedParentSchemaID: keys.PublicSchemaID,
}},
},
{
{ // 11
err: `broken interleave backward reference from "foo"@"bar" to "baz"@"qux"`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -887,7 +971,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
}},
},
{
{ // 12
err: `type ID 500 in descriptor not found: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -910,7 +994,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
},
// Add some expressions with invalid type references.
{
{ // 13
err: `type ID 500 in descriptor not found: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -933,7 +1017,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
},
},
{
{ // 14
err: `type ID 500 in descriptor not found: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -956,7 +1040,7 @@ func TestValidateCrossTableReferences(t *testing.T) {
},
},
},
{
{ // 15
err: `type ID 500 in descriptor not found: descriptor not found`,
desc: descpb.TableDescriptor{
Name: "foo",
Expand All @@ -981,7 +1065,9 @@ func TestValidateCrossTableReferences(t *testing.T) {
}
desc := NewImmutable(test.desc)
if err := desc.ValidateCrossReferences(ctx, descs); err == nil {
t.Errorf("%d: expected \"%s\", but found success: %+v", i, test.err, test.desc)
if test.err != "" {
t.Errorf("%d: expected \"%s\", but found success: %+v", i, test.err, test.desc)
}
} else if test.err != err.Error() && "internal error: "+test.err != err.Error() {
t.Errorf("%d: expected \"%s\", but found \"%s\"", i, test.err, err.Error())
}
Expand Down

0 comments on commit dd23c13

Please sign in to comment.