sql: dropping objects should work even if the objects are invalid

[jordanlewis]

We've begun to see several issues around invalid table descriptor state creeping into the database. This is very troublesome, and we of course would prefer to never have bugs like this in the first place. But, bugs happen. We need to make it easier for people to recover from such situations.

The first reaction of many users upon encountering a "broken seeming" table or object is to try to DROP that object. Our well-intentioned validate methods tend to prevent this from happening, since we try to validate the object before doing anything else.

This issue tracks modifying DROP to care less about validation errors. I think they should still be logged and reported to sentry, and possibly returned to the user as a WARNING, but they shouldn't block the DROP.

A good example of this kind of issue is #50649. The issue prevents the object from being dropped altogether. This is unreasonable - we should obey the user's request and drop the object. Otherwise, the object will enter an unrecoverable state.

[ajwerner]

After further discussion, it's probably not the case that we want to blindly drop tables which are arbitrarily invalid. Rather, we want to support dropping tables which are invalid because they reference objects which do not exist.

We recently had a bug #50587 in which renames would be stuck in an intermediate state. Both the old and new names would be effectively unusable. In this case, there were namespace entries for both the new and old name. The new name works in that case for SELECT but the old name still exists if anybody else tries to rename over it. In that case, one might want to be able to drop the old name. If we didn't do any sanity checking, we might drop data we don't intend to.

[ajwerner]

To make the above example more concrete, with some added back-and-forth renaming you can get in the below situation:

root@localhost:26257/defaultdb> SELECT * FROM foo;                           
pq: relation "foo" does not exist
root@localhost:26257/defaultdb> ALTER TABLE bar RENAME TO foo;
pq: relation "foo" already exists

Where you think you might want to drop table foo which is actually bar. A customer got into this situation and tried to type DROP TABLE foo and I'm pretty happy it did not work.

root@localhost:26257/defaultdb> SELECT * FROM system.namespace where id = 56;               
  parentID | name | id  
+----------+------+----+
        50 | bar  | 56  
        50 | foo  | 56  

[postamar]

I believe this has been done.

[ajwerner]

This has not been done. Maybe we should do something here. In particular, when there's a back-reference to something which does not exist, we may as well ignore it.

[ajwerner]

Relates to #72230.

[postamar]

Perhaps what we really want is for the user to be able to disable validation altogether? This wasn't an option until recently because there was no way to disable validation-on-read while it was too deeply embedded in catalogkv. However, we're now in a place where descriptor reads are mediated by the collection, even if it's only to a minimal degree.

[postamar]

So, after discussing with @ajwerner here's the plan:

• add a session variable that disables validations on read
• in these cases we should be able to use the legacy schema changer with read and write validations disabled, verify this with a handful of logic test cases

[postamar]

I don't know what to do here. We are able to disable validation now but this only helps with being able to run DML queries without complaining about broken back-references which don't matter for queries anyway. However, when DROPping something (and it doesn't matter which schema changer flavour is involved) back-references do matter, and the only way to deal with broken back-references to achieve the result described in the issue description is:

• to add a "permissive" mode to the descs.Collection so that it doesn't choke on the first error it encounters when querying descriptors in bulk, like via GetAllSchemasInDatabase etc.
• to make both the legacy and declarative schema changers invoke this "permissive" mode somehow (not clear how? a session var?) and then to deal with the fact that the catalog API call may not have been successful.

This feels like a lot of work. It might actually be easier to do in the declarative schema changer, but still. I feel that having a builtin that repairs or removes broken cross-references would be more useful. Once we have this, we could look into calling that builtin transparently when a DROP throws a validation error for whatever reason and then retry.

[postamar]

cc @dikshant

[ajwerner]

I'm not convinced that this has been coming as often recently. We're going to defer doing anything. Let's collect evidence that this is important. It's hard. The more we put into making the catalog solid, the less important this will be.