soundcloud / lhm

Online MySQL schema migrations
BSD 3-Clause "New" or "Revised" License
1.83k stars 190 forks source link

Don't choke on lhmn_* in cleanup with range #119

Closed sj26 closed 7 years ago

sj26 commented 9 years ago

When running an Lhm.cleanup with a range it tries to strptime every table name, but if there are unswitched new tables then it fails:

 > Lhm.cleanup(false)

 Existing LHM backup tables: lhmn_statement_lines.
 Existing LHM triggers: .
 Run Lhm.cleanup(true) to drop them all.

 > Lhm.cleanup(true, until: 1.week.ago)

 ...:in `eval': invalid strptime format - `lhma_%Y_%m_%d_%H_%M_%S' (ArgumentError)
    from .../lhm-2.2.0/lib/lhm.rb:64:in `block in cleanup'
    from .../lhm-2.2.0/lib/lhm.rb:63:in `select!'
    from .../lhm-2.2.0/lib/lhm.rb:63:in `cleanup'

This patch includes the table in cleanup over a time range. We can't really date this table, so can't apply the range logic, but it should just be duplicated data in the table so should be safe to delete.

taonic commented 9 years ago

+1 on the fix. But I'm wondering if we are introducing risk by removing lhmn_ all together when the intention of until is to remove lhma_ with the specified date range only.

sj26 commented 9 years ago

Yeah I was wondering that too, @taoza, but this is generated duplicate data, not canonical data:

should just be duplicated data in the table so should be safe to delete

tguo commented 9 years ago

@sj26 Any chance the Lhm.cleanup is triggered while an online migration is in process and lhmn_ is being used to make copy?

sj26 commented 9 years ago

@tguo sure, but if you're running your migrations in multiple places then I think you have bigger problems.

sj26 commented 7 years ago

I'll close this one if there's no movement. :-)