Skip to content

Commit

Permalink
importccl: allow changes to referencing set or privileges on UDT
Browse files Browse the repository at this point in the history
In cockroachdb#69674 we enabled importing of tables which used UDTs. We added the caveat
that these types must not change during the import. In cockroachdb#70987, @ajstorm
uncovered that adding new usages of the type cause an illegal change. This is
a particularly painful limitation as all regional by row tables will use the
enum type of the database. That makes the limitation of import much more
extreme than just precluding renames or modifications of enums or their
members.

To fix this limitation, we permit changes to the referencing set which
occur during the import. We also permit changes to privileges as they
won't impact the correctness of the import.

Relates to cockroachdb#69706
Fixes cockroachdb#70987

Release note (enterprise change): Fixed a limitation of IMPORT for tables
using user-defined types whereby any change to the set of tables or views
which reference the type or any changes to privileges on the type during
the IMPORT would lead to failure. Now new references to the type or GRANT
or REVOKE operations performed while the IMPORT is ongoing will not cause
failure.
  • Loading branch information
ajwerner committed Oct 1, 2021
1 parent 177ed2b commit ce97ef1
Show file tree
Hide file tree
Showing 2 changed files with 45 additions and 5 deletions.
38 changes: 33 additions & 5 deletions pkg/ccl/importccl/import_stmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -2324,6 +2324,29 @@ func (r *importResumer) checkForUDTModification(
if details.Types == nil {
return nil
}
// areEquivalentTypes returns true if a and b are the same types save for the
// version, modification time, privileges, or referencing descriptors.
areEquivalentTypes := func(a, b *descpb.TypeDescriptor) (bool, error) {
clearIgnoredFields := func(d *descpb.TypeDescriptor) *descpb.TypeDescriptor {
d = protoutil.Clone(d).(*descpb.TypeDescriptor)
d.ModificationTime = hlc.Timestamp{}
d.Privileges = nil
d.Version = 0
d.ReferencingDescriptorIDs = nil
return d
}
aData, err := protoutil.Marshal(clearIgnoredFields(a))
if err != nil {
return false, errors.NewAssertionErrorWithWrappedErrf(
err, "failed to marshal existing to check for equivalence")
}
bData, err := protoutil.Marshal(clearIgnoredFields(b))
if err != nil {
return false, errors.NewAssertionErrorWithWrappedErrf(
err, "failed to marshal existing to check for equivalence")
}
return bytes.Equal(aData, bData), nil
}
return sql.DescsTxn(ctx, execCfg, func(ctx context.Context, txn *kv.Txn,
col *descs.Collection) error {
for _, savedTypeDesc := range details.Types {
Expand All @@ -2332,12 +2355,17 @@ func (r *importResumer) checkForUDTModification(
if err != nil {
return errors.Wrap(err, "resolving type descriptor when checking version mismatch")
}
if typeDesc.GetModificationTime() != savedTypeDesc.Desc.GetModificationTime() {
return errors.Newf("type descriptor %d has a different modification time than what"+
" was saved during import planning; unsafe to import since the type"+
" has changed during the course of the import",
typeDesc.GetID())
if typeDesc.GetModificationTime() == savedTypeDesc.Desc.GetModificationTime() {
return nil
}
equivalent, err := areEquivalentTypes(typeDesc.TypeDesc(), savedTypeDesc.Desc)
if equivalent || err != nil {
return err
}
return errors.Newf("type descriptor %d has a different modification time than what"+
" was saved during import planning; unsafe to import since the type"+
" has changed during the course of the import",
typeDesc.GetID())
}
return nil
})
Expand Down
12 changes: 12 additions & 0 deletions pkg/ccl/importccl/import_stmt_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -7286,6 +7286,18 @@ func TestUDTChangeDuringImport(t *testing.T) {
"cannot drop type \"greeting\"",
false,
},
{
"use-in-table",
"CREATE TABLE d.foo (i INT PRIMARY KEY, j d.greeting)",
"",
false,
},
{
"grant",
"CREATE USER u; GRANT USAGE ON TYPE d.greeting TO u;",
"",
false,
},
}

for _, test := range testCases {
Expand Down

0 comments on commit ce97ef1

Please sign in to comment.