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

release-20.1: sql: correctly identify and cleanup dependent objects in drop database #51895

Merged
merged 1 commit into from
Jul 25, 2020

Conversation

arulajmani
Copy link
Collaborator

Backport 1/1 commits from #51813.

/cc @cockroachdb/release


There were a few things going on here because of the way we filtered
tables during DROP DATABASE CASCADE. The intention was to filter
out objects that depended on other objects also being deleted (and would
therefore be deleted by the CASCADE nature of object drops). This
assumption is correct for table-view and view-view dependencies -- but
not for sequences. We also switched from individual schema change jobs
during a database drop to a single job which doesn't play well with
this filtering -- as no new jobs are queued, all objects that were
filtered would leave orphaned namespace/descriptor entries behind.
It's interesting to note that the filtering didn't directly cause this
issue, it just made the underlying issue more visible -- the single drop
database schema change job relies on knowing about all object
descriptors that need to be dropped upfront. Orphaned entries which
would have only occured for cross-database dependencies can occur in
the same database because of the filtering. This leads to why the fix
is what it is.

As part of this patch, I've tried to make the preperation steps for
dropping databases more explicit. First, we accumalate all objects
that need to be deleted. This includes objects that will be implicitly
deleted, such as owned sequences, dependent views (both in the database
being deleted and other databases that aren't being deleted). The
schema change job uses this new list to ensure entries are appropriately
cleaned up. We still perform the filter step as before to identify
objects which are the "root" of a drop and only call drop on these
objects. The only change here is that instead of accumulating dependent
objects, we explicitly accumulate cascading views.

Fixes #51782
Fixes #50997

Release note (bug fix): Before this change, we would leave orphaned
system.namespace/system.descriptor entries if we ran a
DROP DATABASE CASCADE and the database contained "dependency"
relations. For example, if the database included a view which
depended on a table in the database, dropping the database would result
in an orphaned entry for the view. Same thing for a sequence that was
used by a table in the database. (See #51782 for repro steps). This bug
is now fixed, and cleanup happens as expected.

There were a few things going on here because of the way we filtered
tables during `DROP DATABASE CASCADE`. The intention was to filter
out objects that depended on other objects also being deleted (and would
therefore be deleted by the CASCADE nature of object drops). This
assumption is correct for table-view and view-view dependencies -- but
not for sequences. We also switched from individual schema change jobs
during a database drop to a single job which doesn't play well with
this filtering -- as no new jobs are queued, all objects that were
filtered would leave orphaned namespace/descriptor entries behind.
It's interesting to note that the filtering didn't directly cause this
issue, it just made the underlying issue more visible -- the single drop
database schema change job relies on knowing about all object
descriptors that need to be dropped upfront. Orphaned entries which
would have only occured for cross-database dependencies can occur in
the same database because of the filtering. This leads to why the fix
is what it is.

As part of this patch, I've tried to make the preperation steps for
dropping databases more explicit. First, we accumalate all objects
that need to be deleted. This includes objects that will be implicitly
deleted, such as owned sequences, dependent views (both in the database
being deleted and other databases that aren't being deleted). The
schema change job uses this new list to ensure entries are appropriately
cleaned up. We still perform the filter step as before to identify
objects which are the "root" of a drop and only call drop on these
objects. The only change here is that instead of accumulating dependent
objects, we explicitly accumulate cascading views.

Fixes cockroachdb#51782
Fixes cockroachdb#50997

Release note (bug fix): Before this change, we would leave orphaned
system.namespace/system.descriptor entries if we ran a
`DROP DATABASE CASCADE` and the database contained "dependency"
relations. For example, if the database included a view which
depended on a table in the database, dropping the database would result
in an orphaned entry for the view. Same thing for a sequence that was
used by a table in the database. (See cockroachdb#51782 for repro steps). This bug
is now fixed, and cleanup happens as expected.
@arulajmani arulajmani requested a review from ajwerner July 24, 2020 23:32
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner)

@arulajmani arulajmani merged commit 6af1d20 into cockroachdb:release-20.1 Jul 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants