Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: refactor drop table cascade to reduce potential for bugs going forward #51916

Open
arulajmani opened this issue Jul 27, 2020 · 2 comments
Open
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)

Comments

@arulajmani
Copy link
Collaborator

arulajmani commented Jul 27, 2020

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

This is in context to #51813 to track follow up work that makes future bugs with DROP TABLE CASCADE less likely. The main motivation here is #51782 and #50997.

It isn't ideal that we do a recursive accumulation of objects that will be dropped in the preparation phase of DROP DATABASE CASCADE because this implicitly happens when we execute drops on individual objects. This is currently unavoidable because:

  • We allow cross database references, so simply scanning the namespace table doesn't provide the full picture -- we need to recursively use the "dependsOn" field on the descriptor.

  • We use this list of objects to filter out dependent objects which will be cascade deleted. This is more than just an optimization, as the individual object drop implementations expect dependent descriptors to not be dropped.

  • The drop database job is responsible for schema changes on all dropped objects (instead of individual objects having their own jobs). If the job doesn't know of all the objects that are being dropped, it will leave behind dangling namespace/descriptor entries and data.

Describe the solution you'd like

  • Change dropTableImpl,dropSequenceImpl and dropViewImpl to return a list of objects dropped instead of a list of cascading views. This would include the object itself in addition to any owned sequences and dependant views.

  • When we implement DROP TYPE CASCADE this list should also include dropped types as well. Types for the most part will play well with DROP DATABASE CASCASDE as we don't allow cross-database type references. But the problem is pushed one level down to DROP SCHEMA instead, as we do allow cross-schema type references, and presumably DROP SCHEMA would have a similar structure to the one DROP DATABASE does today.

  • Instead of pre-fetching descriptors during the planning phase , we should restrict the planning phase to getting entries from the namespace table and let execution get descriptors as and when we are about to call drop on an object. This allows us to check for dropped descriptors (which are dropped because of a cascade delete) and simply skip over them. This is much cleaner than special casing for dropped descriptors in individual object drop implementations, which can get a bit confusing

Jira issue: CRDB-3996

@blathers-crl blathers-crl bot added the C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. label Jul 27, 2020
@arulajmani arulajmani self-assigned this Jul 27, 2020
@arulajmani arulajmani added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. and removed C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. labels Jul 27, 2020
@arulajmani
Copy link
Collaborator Author

@ajwerner typed this up, would love to get some thoughts here!

@cockroachdb cockroachdb deleted a comment from blathers-crl bot Jul 28, 2020
@thoszhang thoszhang added this to Triage in SQL Foundations via automation Aug 26, 2020
@jordanlewis
Copy link
Member

cc @ajwerner

@jordanlewis jordanlewis moved this from Triage to Backlog in SQL Foundations Sep 1, 2020
@rafiss rafiss added this to Triage in SQL Sessions - Deprecated via automation Oct 27, 2020
@rafiss rafiss moved this from Triage to Shorter term backlog in SQL Sessions - Deprecated Oct 27, 2020
@rafiss rafiss added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label May 12, 2021
@jlinder jlinder added the T-sql-schema-deprecated Use T-sql-foundations instead label Jun 16, 2021
@postamar postamar moved this from Backlog to Declarative Schema Changer Graveyard in SQL Foundations Mar 11, 2022
@exalate-issue-sync exalate-issue-sync bot removed the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Aug 31, 2022
@rafiss rafiss removed this from Shorter term backlog in SQL Sessions - Deprecated Sep 9, 2022
@exalate-issue-sync exalate-issue-sync bot added T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) and removed T-sql-schema-deprecated Use T-sql-foundations instead labels May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions)
Projects
SQL Foundations
  
Declarative Schema Changer Will Fix T...
Development

No branches or pull requests

4 participants