importccl: fail imports on concurrent type changes less aggressively

[adityamaru]

Following #69674, an import into job will store the set of type descriptors referenced by the table being imported into during planning. On completion, the import job will check that the modification time on the saved type descriptors is equal to that on the type descriptors that are looked up. If there is a mismatch, it means that there was a concurrent type change during the import, and so we fail the import job. The method performing this check is called checkForUDTModification.

This check is probably too aggressive and risks failing an import at the last stage unnecessarily. The semantics of which concurrent type changes are safe during an import is still to be ironed out. Once that is done, this method can be refined to only check for the occurrence of the unsafe type changes and fail the import job. Some of the discussions around classes of type changes are listed below:

• DROP TYPE: We forbid a type from being dropped if there is a table referencing it. This does not change the modification time on the descriptor and so the import job will succeed today.

• ALTER TYPE DROP VALUE: We forbid an enum value from being dropped if a table referencing it is being imported into. The method canRemoveEnumValue validates all tables referencing the type and fails with could not validate enum value removal for "hi": relation "t" is offline: importing. This type change however changes the modification time prior to hitting the above error, and thus fails the import job today.

• ALTER TYPE ADD VALUE: Adding a value to an enum being imported into should probably be safe. Today it will fail the import job since the modification time changes.

• ALTER TYPE RENAME VALUE: Semantics to be thought through.

• ALTER TYPE RENAME TO: Semantics to be thought through.

Jira issue: CRDB-9734

[ajwerner]

Following #69674, an import into job will store the set of type descriptors referenced by the table being imported into during planning.

Do we need to store the set of type descriptors explicitly? The table descriptors know which type descriptors they reference.

DROP TYPE: We forbid a type from being dropped if there is a table referencing it. This does not change the modification time on the descriptor and so the import job will succeed today.

What do you mean that the modification time does not change? Generally, I don't get this one. Don't we end up forbidding the type from being dropped because the importing descriptor references the type?

[adityamaru]

Do we need to store the set of type descriptors explicitly? The table descriptors know which type descriptors they reference.

For IMPORT INTO to support default + computed columns referencing UDTs we will be adding a custom type resolver that will be used during import execution. This type resolver will be backed by this set of cached type descriptors, and so I thought it necessary to store the descriptors instead of just the ModificationTime or a subset of the fields.

Don't we end up forbidding the type from being dropped because the importing descriptor references the type?

Yes, we do, so the import job is unaffected. What I was trying to get at was that while DROP TYPE is forbidden and does not change the descriptors modification time, ALTER TYPE DROP VALUE is forbidden, but does update the modification time. Thus, in the latter case, we do fail the import job rather unnecessarily. I am still to look into where ALTER TYPE DROP VALUE changes the modification time before it errors out in canRemoveEnumValue.

[ajwerner]

This type resolver will be backed by this set of cached type descriptors, and so I thought it necessary to store the descriptors instead of just the ModificationTime or a subset of the fields.

It's not a bad idea. Otherwise you'd need to go read the historical descriptors. Makes sense.

Yes, we do, so the import job is unaffected. What I was trying to get at was that while DROP TYPE is forbidden and does not change the descriptors modification time, ALTER TYPE DROP VALUE is forbidden, but does update the modification time.

Got it, so can we check off the first box?

I am still to look into where ALTER TYPE DROP VALUE changes the modification time before it errors out in canRemoveEnumValue.

This seems tricky.

[adityamaru]

Adding @arulajmani's comment about why ALTER TYPE DROP VALUE updates the modification time even though it is forbidden.

I think it's because the OFFLINE error that you're expecting comes from the async portion of the schema change. The user transaction moves the enum member from PUBLIC to READ ONLY which results in an update to the modification time. The error you're expecting happens when we validate the enum member usages -- we can't for this table because it is offline. Validation fails and the enum member is transitioned back to PUBLIC from READ ONLY, resulting in another update to the modification time.

[ajwerner]

We have decided that this is very low priority. There is quite a bit of work to unify import and sql #67076. There's also quite a bit of work to fix up types. This is an edge case for import.