-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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-23.1: prevent data loss for at risk indexes and add tooling to detect them #106863
Conversation
… a column Previously, because of a bug we could end up with secondary indexes being the only thing that stored a column. This could lead to data loss if that secondary index was dropped. To address this, this patch will block dropping such indexes with a link to the technical advisory. Informs: cockroachdb#106739 Release note (bug fix): Block dropping of indexes impacted by technical advisory 99561 if data loss could occur.
Thanks for opening a backport. Please check the backport criteria before merging:
If some of the basic criteria cannot be satisfied, ensure that the exceptional criteria are satisfied within.
Add a brief release justification to the body of your PR to justify this backport. Some other things to consider:
|
2f3e694
to
257c252
Compare
…olumns Previously, we had a bug were it was possible that the primary indexes was not storing some non-virtual columns. Unfortunately, adding validation in all cases would break existing customers, so as a safety we have added validation to crdb_internal.invalid_objects and the debug doctor command. To address this, this patch adds a new validation level only for invalid_objects / doctor. Fixes: cockroachdb#106794 Informs: cockroachdb#106739 Release note: None
257c252
to
6d7c916
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is quite nice! just had a minor question about the tests
also, do you think we should forward-port the first commit to master
?
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @fqazi)
pkg/sql/logictest/testdata/logic_test/drop_index
line 558 at r1 (raw file):
# Drop INDEX will be blocked statement error pq: index t_with_idx_data_loss_n_idx cannot be safely dropped, since doing so will lose data in certain columns DROP INDEX t_with_idx_data_loss@t_with_idx_data_loss_n_idx;
nice test here. just wanted to check if the test already runs for both legacy and new schema changers?
That scenario is already blocked by validation on master, so we should no longer have the danger anymore.
Yes this test runs in both cases, so we should be covered |
Encountered an error creating backports. Some common things that can go wrong:
You might need to create your backport manually using the backport tool. error creating merge commit from 6c8bd9a to blathers/backport-release-22.2-106863: 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 dev-inf. |
This patch aims at doing the following:
Fixes: #106794
Informs: #106739
Release justification: low risk and prevents a potentially dangerous action by users