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/gc_job,sqlerrors: make GC job robust to missing descriptors #99665

Merged

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Mar 27, 2023

sql: do not drop table descriptor independently if we're in drop schema

If we have dropped schema IDs, we know that this is not an individual drop table
schema change. We only have more than one dropped table when we drop a database
or a schema. Before this change, we'd drop the table on its own, and then create
another GC job to drop all the tables. This is not actually a bug because we
should be robust to this, but it's also bad.

sql/gc_job,sqlerrors: make GC job robust to missing descriptors

The check used for missing descriptors became incorrect in the course of
#94695. That change updated
the underlying error code used in getters by the GC job. The GC job would
subsequently retry forever when the descriptor was missing. This bug
has not been shipped yet, so not writing a release note.

Fixes: #99590
Fixes: #99665

Release note (bug fix): DROP SCHEMA ... CASCADE could create multiple
GC jobs: one for every table and one for the cascaded drop itself. This has
been fixed.

If we have dropped schema IDs, we know that this is not an individual drop table
schema change. We only have more than one dropped table when we drop a database
or a schema. Before this change, we'd drop the table on its own, and then create
another GC job to drop all the tables. This is not actually a bug because we
should be robust to this, but it's also bad.

Epic: none

Release note (bug fix): DROP SCHEMA ... CASCADE could create multiple
GC jobs: one for every table and one for the cascaded drop itself. This has
been fixed.
@ajwerner ajwerner added the backport-23.1.x Flags PRs that need to be backported to 23.1 label Mar 27, 2023
@ajwerner ajwerner requested review from a team March 27, 2023 15:29
@cockroach-teamcity
Copy link
Member

This change is Reviewable

The check used for missing descriptors became incorrect in the course of
cockroachdb#94695. That change updated
the underlying error code used in getters by the GC job. The GC job would
subsequently retry forever when the descriptor was missing. This bug
has not been shipped yet, so not writing a release note.

Fixes: cockroachdb#99590

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/fix-gcjob-with-deleted-descriptors branch from e386b69 to fb2a3cc Compare March 27, 2023 16:30
@ajwerner
Copy link
Contributor Author

The test failure is unrelated: TestCreateComputedIndexesOnSystemSQLStatistics

Copy link
Collaborator

@fqazi fqazi left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 1 of 1 files at r2, 8 of 8 files at r3, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

@ajwerner
Copy link
Contributor Author

TFTR!

bors r=fqazi

@craig
Copy link
Contributor

craig bot commented Mar 27, 2023

Build succeeded:

@blathers-crl
Copy link

blathers-crl bot commented Mar 27, 2023

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.


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


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-23.1.x Flags PRs that need to be backported to 23.1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

roachtest: schemachange/mixed-versions failed
3 participants