importccl: allow changes to referencing set or privileges on UDT

[ajwerner]

In #69674 we enabled importing of tables which used UDTs. We added the caveat
that these types must not change during the import. In #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 #69706
Fixes #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.

[cockroach-teamcity]

This change is 

[ajstorm]

This PR made my day 🎉. Some comments below.

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @ajwerner)

---

pkg/ccl/importccl/import_stmt.go, line 2362 at r1 (raw file):

			}
			equivalent, err := areEquivalentTypes(typeDesc.TypeDesc(), savedTypeDesc.Desc)
			if equivalent || err != nil {

Nit: Might just be me, but I find this reads better as:

if err != nil {
   return err
}
if equivalent {
   return nil
}

inferring that err == nil when equivalent == true takes a bit of reasoning which slows down the reading here.

---

pkg/ccl/importccl/import_stmt.go, line 2365 at r1 (raw file):

				return err
			}
			return errors.Newf("type descriptor %d has a different modification time than what"+

I think this message needs updating to describe that the type has fundamentally changed.

---

pkg/ccl/importccl/import_stmt_test.go, line 7295 at r1 (raw file):

			false,
		},
		{

Nit: Is it possible to modify TestImportMultiRegion to add some concurrency and validate that the scenario which originally flagged this issue is resolved (and stays resolved)?

[ajwerner]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru and @ajstorm)

---

pkg/ccl/importccl/import_stmt.go, line 2362 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Might just be me, but I find this reads better as:

if err != nil {
   return err
}
if equivalent {
   return nil
}

inferring that err == nil when equivalent == true takes a bit of reasoning which slows down the reading here.

Done.

---

pkg/ccl/importccl/import_stmt.go, line 2365 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

I think this message needs updating to describe that the type has fundamentally changed.

Done.

---

pkg/ccl/importccl/import_stmt_test.go, line 7295 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Nit: Is it possible to modify TestImportMultiRegion to add some concurrency and validate that the scenario which originally flagged this issue is resolved (and stays resolved)?

Done.

[ajstorm]

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru)

---

pkg/ccl/importccl/import_stmt.go, line 2365 at r1 (raw file):

Previously, ajwerner wrote…

Done.

Looks good. One more think you might want to think of adding here is a hint. Something like "Retrying the IMPORT operation may succeed if the operation concurrently modifying the descriptor does not reoccur during the retry attempt."

Super nitty comment: I think you might want to add commas before and after "potentially incompatibly".

[ajstorm]

Added backport flag to 21.2. IMO, this is a candidate for 21.2.1, but not a GA blocker.

[ajwerner]

Reworked the control flow as an early return would have allowed changes to subsequent types to slip through due to a return rather than a continue. Fortunately the linter caught that. Added a test and changed things around to avoid any need for continue.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @ajstorm)

---

pkg/ccl/importccl/import_stmt.go, line 2365 at r1 (raw file):

Previously, ajstorm (Adam Storm) wrote…

Looks good. One more think you might want to think of adding here is a hint. Something like "Retrying the IMPORT operation may succeed if the operation concurrently modifying the descriptor does not reoccur during the retry attempt."

Super nitty comment: I think you might want to add commas before and after "potentially incompatibly".

Done.

[ajwerner]

TFTR!

bors r+

[craig]

Build succeeded:

• GitHub CI (Cockroach)