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/schemachanger: reassign owned by and concurrent declarative schema changer drops can conflict #93101

Merged
merged 3 commits into from
Dec 7, 2022

Conversation

fqazi
Copy link
Collaborator

@fqazi fqazi commented Dec 6, 2022

Informs: #87572

A number of bugs exist inside DROP OWNED / REASSIGNED OWNED BY inside the declarative schema changer:

  1. If a concurrent DROP operation for schema/database with a REASSIGNED OWNED BY could lead to the descriptors being dropped incorrectly by legacy schema changer jobs. This patch adds additional logic to ensure that only descriptors marked for drops are cleaned up.
  2. The DROP OWNED BY support inside the declarative schema changer requires no privileges to be defined, unfortunately, this logic did not have proper error handling
  3. When cleaning up back-references from schemas to the database, we skipped certain operations if the database was dropped, which is true after any cascaded DROP DATABASE statement phase. Which can lead to other issues depending on the stages executed.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@fqazi fqazi force-pushed the txnRetryError branch 5 times, most recently from 3b8e946 to 3c18951 Compare December 6, 2022 14:25
@fqazi fqazi marked this pull request as ready for review December 6, 2022 15:21
@fqazi fqazi requested a review from a team December 6, 2022 15:21
Copy link
Contributor

@postamar postamar left a comment

Choose a reason for hiding this comment

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

Nice!

pkg/sql/schema_changer.go Outdated Show resolved Hide resolved
fqazi added 3 commits December 7, 2022 15:39
…ptors

Previously, the legacy schema changer would create a job
to clean up database/schema descriptors, however it would only
care if the descriptor was marked as dropped. This was
inadequate because with the declarative schema changer
becoming in use, its possible for it to interfere with
the declarative schema changer, if a legacy schema changer
job is kicked off after the pre-commit phase of a declarative
drop. To address this, the legacy schema changer now checks
if the descriptor ID is in the job details.

Release note: None
Previously, we had logic to disable the declarative schema changer
for drop owned by if the user had any privileges in the
system.privilege table. Unfortunately, the error handling
for this logic was broken. This patch, properly checks
for errors in this code.

Release note: None
Previously, when a database descriptor was labeled as dropped,
we didn't bother cleaning up the schema entry inside. This could
be problematic in certain resolution code paths where we will
resolve databases by name even if they are even dropped, which
can cause these paths to hit a missing descriptor error. The
patch addresses, this bug by modifying dropped descriptors
inside the decalrative schema changer during execution.

Release note: None
@fqazi
Copy link
Collaborator Author

fqazi commented Dec 7, 2022

@postamar TFTR!

@fqazi
Copy link
Collaborator Author

fqazi commented Dec 7, 2022

bors r+

@craig
Copy link
Contributor

craig bot commented Dec 7, 2022

Build succeeded:

@craig craig bot merged commit a9fcbd1 into cockroachdb:master Dec 7, 2022
@blathers-crl
Copy link

blathers-crl bot commented Dec 7, 2022

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


error creating merge commit from 37df598 to blathers/backport-release-22.2-93101: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 22.2.x failed. See errors above.


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

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