Fix leaked bp_oximeter_read_policy rows
#8577
Conversation
| @@ -0,0 +1,3 @@ | |||
| set local disallow_full_table_scans = off; | |||
|
|
|||
| DELETE FROM bp_oximeter_read_policy WHERE blueprint_id NOT IN (SELECT blueprint_id FROM blueprint); | |||
There was a problem hiding this comment.
This looks correct to me, but I'll defer to my local sql expert @jgallagher and also recommend a test if possible.
There was a problem hiding this comment.
I basically just copying @davepacheco's sql from the issue #8437 😄 . I'm not sure where I could add a test for this? Truthfully, I wouldn't want to spend too much time on this, as it's not a huge priority? WDYT?
There was a problem hiding this comment.
The SQL looks right to me at a glance, but a migration that just deletes data seems dicey enough to warrant a test, I think? We have several migration tests already, so it's not as painful as it sounds: write a "before" (that inserts some data, probably some blueprint rows and then some bp_oximeter_read_policy rows, some of which match blueprints you inserted and some of which don't) and an "after" (that checks that the not-associated-with-a-blueprint rows were deleted and the others were kept), then add it to get_migration_checks(). Just skimming it looks like {before,after}_148_0_0 is pretty similar: before inserts some rows, then the migration runs, then after confirms the contents.
There was a problem hiding this comment.
I agree with @jgallagher that for a delete operation we need some assurance we don't wipe unnecessary data. His suggestion was almost exactly what I was going to recommend. Even if only done manually it would probably be fine. Since we have these migration tests, we may as well add a new one. It's probably easier to write and run one of those than do a manual test anyway :)
There was a problem hiding this comment.
Nice! I wasn't aware that there were some integration tests for migrations. Thanks for not letting me be lazy 😅 . All done!
Closes #8437