tabledesc: fix the referencing_descriptor_ids field in type descriptors

[postamar]

There are a few issues related to ReferencingDescriptorIDs in type descriptors, all related to how these descriptor references behave differently than others, like foreign key references, etc.

1. Although the name would suggest otherwise, ReferencingDescriptorIDs only contains table descriptor IDs. Either rename the field to reflect this, or also include database or type descriptor IDs as well.

2. Unlike all other descriptor references, ReferencingDescriptorIDs contains not just direct references but also transitive references. Indeed, when a table descriptor goes through the schema changer machinery, we add its ID to all type descriptors whose IDs are in the return value of GetAllReferencedTypeIDs(), which includes the type descriptors' transitive closure. This is useful to quickly check whether a type can be dropped safely or not, but it is unlikely to scale well and has all kinds of ugly implications (what happens when we have record types as well as enum types? etc.). I strongly feel we should stick to direct references, along with providing some useful machinery in the descriptor collection or wherever in catalog to walk the relational graph to collect all transitive dependencies.

3. Unlike all other descriptor references, there are no back-reference checks done on ReferencingDescriptorIDs when validating a table descriptor, for instance. Adding these is not so straightforward, since in some cases these back-references are not added in the same transaction but asynchronously in a schema changer job. Specifically, I tried adding back-reference checks and the CCL logic test suite alter_table_locality failed with the multiregion-9node-3region-3azs config due to a validation failure following a ROLLBACK.

To address these points I suggest we apply a similar treatment to this field as we did to the old foreign key representation:

• deprecate the referencing_descriptor_ids field in the type descriptor proto;
• introduce new fields in the protos (referencing_database_descriptor_ids, referencing_table_descriptor_ids, referencing_type_descriptor_ids) containing only direct references;
• provide a schema migration to migrate from the old field to the new;
• add an extra step in the RunPostDeserializationChanges type descriptor builder method implementation to do the same thing;
• add a method to the descriptor collection which, given a type descriptor ID, builds the set of transitive dependencies for that type, by walking through the dependency graph, effectively rebuilding referencing_descriptor_ids;
• add a generous quantity of test cases, especially involving backups, so that we can make this change with confidence.

Jira issue: CRDB-6452

Epic CRDB-31469

[arulajmani]

Indeed, when a table descriptor goes through the schema changer machinery, we add its ID to all type descriptors whose IDs are in the return value of GetAllReferencedTypeIDs(), which includes the type descriptors' transitive closure.

I was in the backup/restore area debugging something entirely different when I chanced upon an explanation for why this may have been designed the way it was. Consider the case when we're backing up a table that contains a user defined type. If this table is restored into a database which doesn't contain this type we also restore the type descriptor. Note that a type descriptor can't exist without a corresponding array type alias. As such, it looks like we elected to capture the array type descriptor as a "dependency". This if block

cockroach/pkg/ccl/backupccl/restore_planning.go

Line 732 in 1684499

if desc == nil {

. is where all this happens.

FWIW, I don't think we necessarily need to rely on backing up the array type descriptor for this though, considering that the array type descriptor can be entirely derived from the actual enum type descriptor.

[postamar]

Consider the case when we're backing up a table that contains a user defined type. If this table is restored into a database which doesn't contain this type we also restore the type descriptor. Note that a type descriptor can't exist without a corresponding array type alias. As such, it looks like we elected to capture the array type descriptor as a "dependency".

Thanks for the explanation! This design choice makes a lot more sense to me now.

[postamar]

This just got pushed onto the critical path, if we're to implement DROP TYPE ... CASCADE correctly. Also bad things like #84305 keep happening because of this lack of validation, so we might as well do it sooner rather than later.

[postamar]

I've unassigned myself considering that I'm very unlikely to close this before going on leave.