cockroachdb / cockroach

CockroachDB — the cloud native, distributed SQL database designed for high availability, effortless scale, and control over data placement.
https://www.cockroachlabs.com
Other
30.18k stars 3.82k forks source link

Ability to prevent dropping an index when a column referencing the index is dropped #73445

Open lancel66 opened 2 years ago

lancel66 commented 2 years ago

Is your feature request related to a problem? Please describe.

Currently, dropping a column that is referenced by an index (either directly or via STORING) will silently drop the index as well. Although this is described in the CockroachDB documentation, it is considered dangerous by a customer who runs a large on-prem microservice environment where they operate DBaaS for internal customers.

Describe the solution you'd like

Provide the ability not to drop the index on an opt-in basis (either via a cluster/session setting or CASCADE)

Describe alternatives you've considered

N/A

Jira issue: CRDB-11592

blathers-crl[bot] commented 2 years ago

Hello, I am Blathers. I am here to help you get the issue triaged.

I was unable to automatically find someone to ping.

If we have not gotten back to your issue within a few business days, you can try the following:

:owl: Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.

rafiss commented 2 years ago

cc sql-schema (@postamar / @vy-ton) for triage, since it seems like this request comes from a customer.

postamar commented 2 years ago

Improving DROP COLUMN's behaviour doesn't sound unreasonable. I have to admit, adding more session vars beyond sql_safe_updates seems distasteful to me at first glance, plus you can't expect this statement to not have side effects.

I'd be curious to learn how this is handled in other RDBMSs, to better understand what the expected behaviour is. From what I can guess, the request would be to have a mode where dropping columns is allowed but only if the only thing referencing it is the table and its primary index. Presence of the column in a FK, secondary index, constraint or expression of any kind would cause failure. That is to say, unless CASCADE is specified.

This would be reasonable. I'm curious if users would also expect to be forbidden to drop columns in tables that are not empty? There's a wide range of potentially reasonable intermediate behaviours between the current two extremes of "you can't remove a column at all" and "nuke the column and everything relating to it".

edit: also to what extent are we bound by current user expectations as to what our DROP COLUMN statement does?

rafiss commented 2 years ago

I'd be curious to learn how this is handled in other RDBMSs

Postgres drops the column and any indexes that reference it without a warning or requiring CASCADE. We do try to stick to Postgres-compatibility, but of course we also do intentionally diverge from it if necessary.

to what extent are we bound by current user expectations as to what our DROP COLUMN statement does?

This would definitely be a backwards-incompatible change, since statements that previously would not error would now start erroring. That wouldn't be good for users who use this kind of statement in testing automation. I guess what I'm saying is: if we do change it, it should be an intentional change that we're sure about in a broader sense.

ajwerner commented 2 years ago

My sense is that postgres compatibility wins here. We could issue a notice. I'm not inclined to do anything more. I defer to @vy-ton.

lancel66 commented 2 years ago

Comment from customer @vy-ton:

Developers have used the duplicate index pattern, and have covering indexes on all indexes in order to leverage that pattern fully. They then drop a column, and it wipes out every single secondary index. This is a problem in their environment.

Even without duplicate index pattern, developers are very much at risk of dropping a column and having that drop an index implicitly. We don't consider that behavior acceptable

This behavior doesn't follow the principle of least surprise

MySQL and SQL Server don't do this

Understood that this would diverge from PostgreSQL compatibility which is why they are suggesting an opt-in option. Then compatibility will be maintained by default.

vy-ton commented 2 years ago

fyi I'm thinking about this use case and will get back asap

vy-ton commented 2 years ago

We won't be changing anything for the 22.1 roadmap but will continue to evaluate this request in future planning. FYI @devadvocado