-
Notifications
You must be signed in to change notification settings - Fork 4k
sql/schemachanger: fix incorrect filter for pk index swaps #154752
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
base: master
Are you sure you want to change the base?
Conversation
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
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.
LGTM! I am wondering if a dml injection would have caught this sooner? Maybe by scanning the secondary index at each stage of an ALTER PK.
Previously, we adjusted the schema changer rules to ensure that old secondary indexes are only dropped when the new secondary index is usable. Unfortunately, one of the rules had an incorrect filter. To address this, this patch fixes the filter to expect the new secondary index, which will have the new flag. Additionally, the index recreation logic for ALTER PRIMARY KEY was delaying when the new secondary indexes could be made public. Fixes: cockroachdb#154751 Release note: None
@spilchen TFTR! bors r+ |
154684: backup: split up the multiregion datadriven test r=jeffswenson a=jeffswenson This splits up the multiregion datadriven test so that each test has at most 2 clusters in it. We've been seeing some stuck server shutdowns and this should make them easier to troubleshoot. Release note: none Informs: #145079 154687: backup: improve datadriven test cleanup r=jeffswenson a=jeffswenson Previously, the datadriven test harness would tear down clusters in order. This makes it difficult to troubleshoot stuck tear downs because there are goroutines for a running server mixed in with goroutines for a server with a stuck shutdown. Release note: none Informs: #145079 154752: sql/schemachanger: fix incorrect filter for pk index swaps r=fqazi a=fqazi Previously, we adjusted the schema changer rules to ensure that old secondary indexes are only dropped when the new secondary index is usable. Unfortunately, one of the rules had an incorrect filter. To address this, this patch fixes the filter to expect the new secondary index, which will have the new flag. Additionally, the index recreation logic for ALTER PRIMARY KEY was delaying when the new secondary indexes could be made public. Fixes: #154751 Release note: None 154867: go.mod: bump datadriven r=RaduBerinde a=RaduBerinde Bump datadriven to incorporate a fix (cockroachdb/datadriven#60). Epic: none Release note: None 154870: changefeedccl: make bulk delivery of rangefeed events optional r=aerfrei a=asg0451 This is a temporary opt-out until we can properly test the performance impact of bulk delivery. Epic: none Release note (general change): The changefeed bulk delivery setting was made optional. Co-authored-by: Jeff Swenson <jeffswenson@betterthannull.com> Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com> Co-authored-by: Radu Berinde <radu@cockroachlabs.com> Co-authored-by: Miles Frankel <miles.frankel@cockroachlabs.com>
This PR was included in a batch that was canceled, it will be automatically retried |
Previously, we adjusted the schema changer rules to ensure that
old secondary indexes are only dropped when the new secondary index is
usable. Unfortunately, one of the rules had an incorrect filter. To
address this, this patch fixes the filter to expect the new secondary
index, which will have the new flag. Additionally, the index recreation
logic for ALTER PRIMARY KEY was delaying when the new secondary indexes
could be made public.
Fixes: #154751
Release note: None