-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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-22.2: upgrades: add missing statement_diagnostics_requests check #96220
release-22.2: upgrades: add missing statement_diagnostics_requests check #96220
Conversation
The migration written for system.statement_diagnostics_requests to support probabilistic collection forgot to add the check constraint. Unfortunately, the roachtest to ensure that the system database after the upgrade matches the bootstrapped schema was itself broken for a while, so we missed it. It's not quite clear what to do for existing clusters which have been upgraded, but at least we can fix it for future upgrades. Maybe we'll need to write a migration such that 23.1 adds the check if it does not exist. Epic: none Fixes: cockroachdb#95503 Release note (bug fix): Fixed a bug whereby a system check constraint on statement_diagnostics_requests was not properly added when upgrading from the previous release.
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:
|
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. |
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.
Sorry for introducing the bug, this was silly! We have this defensive code that checks for the right bounds before doing the row insert. The only "public" way to insert rows into this table is to use crdb_internal.request_statement_bundle
. So unless a user has mucked with the table directly, perhaps we can assume that the contents of the table are sound/abiding the check constraint that we failed to add for upgrades into 22.2.{1-3}. All of which is to say that "Maybe we'll need to write a migration such that 23.1 adds the check if it does not exist" sounds good to me. You've already done all the work with checkConstraintExists
-- do you want send the 23.1 patch or should I?
cockroach/pkg/sql/stmtdiagnostics/statement_diagnostics.go
Lines 292 to 296 in 2f81142
if samplingProbability < 0 || samplingProbability > 1 { | |
return 0, errors.Newf( | |
"expected sampling probability in range [0.0, 1.0], got %f", | |
samplingProbability) | |
} |
If you're willing, that'd be great |
The migration written for system.statement_diagnostics_requests to support probabilistic collection forgot to add the check constraint. Unfortunately, the roachtest to ensure that the system database after the upgrade matches the bootstrapped schema was itself broken for a while, so we missed it.
It's not quite clear what to do for existing clusters which have been upgraded, but at least we can fix it for future upgrades.
Maybe we'll need to write a migration such that 23.1 adds the check if it does not exist.
Epic: none
Release justification: fixes a bug in a migration
Fixes: #95503
Release note (bug fix): Fixed a bug whereby a system check constraint on statement_diagnostics_requests was not properly added when upgrading from the previous release.