Remove nextrun index #107
Remove nextrun index #107
Conversation
![hm-linter[bot]](https://avatars.githubusercontent.com/in/5455?s=60&v=4){kind=small}
| * | ||
| * Remove nextrun index as it negatively affects performance. | ||
| */ | ||
| function upgrade_database_4() { |
There was a problem hiding this comment.
Can we also update the v3 alter command so that someone migrating from v2 -> v4 doesn't get a useless index in the meantime?
|
I think although it's a database change there's no functional change and it solves a problem so I suggest we do this as a patch release. |
| global $wpdb; | ||
|
|
||
| $query = "ALTER TABLE `{$wpdb->base_prefix}cavalcade_jobs` | ||
| DROP INDEX `nextrun`"; |
There was a problem hiding this comment.
Does this need an IF EXISTS or something too?
There was a problem hiding this comment.
@rmccue well, it doesn't need it, but this will mean that the ALTER TABLE statement will be executed no matter what. It's not a big deal, but also one of the reasons why I didn't change the v3 statement. I also thought that keeping old versions (even if this means unnecessarily introducing an intermediary index) as expected would be a good thing.
MySQL does not support IF EXISTS for DROP INDEX—MariaDB does, though.
So, we would need to run a query to check for the index, and if present, drop it. That's not really any better than running the above quick and meaningless ALTER TABLE query, but the difference would be to only run a write query if necessary.
There was a problem hiding this comment.
Following up on this, just to clarify the "not a big deal": MySQL will throw an error for trying to drop an index that does not exist. But WordPress/PHP will not complain (and work just fine) as this is, fortunately, the only thing that this query should do. wp cavalcade upgrade will execute successfully.
Personally, I think it best to change the v3 statement back to what it was initially. If there were tests for the individual upgrade routines, or tests making use of them to prepare the test data/context, they would have needed to be changed. Would we have done/wanted that?
There was a problem hiding this comment.
Maybe, we don't have tests for the upgrade routines themselves as that's quite difficult to do. Easier to let the initial set up happen and test the core functionality.
If upgrades happen without a hitch still then I think changing anything now isn't really much of a priority. We could add tests for the upgrade routines in future but it feels like high effort for low return. The database creation and upgrade process is not tested individually but as a whole in the sense it needs to work for the rest of the tests to pass which is good enough right now.
There was a problem hiding this comment.
@roborourke that's not what I meant. I wasn't asking for tests. You were able to "easily" adapt the v3 statement because there were no tests that broke. I was just questioning if we would have changed anything in a previous version if there would have been tests (or other kind of dependent code) that broke. 😉
There was a problem hiding this comment.
I think it depends on what is being changed so there's no generic answer for me. I guess you're saying that it's bad practice (which is totally fair!) and we should change the v3 database step back, but at least in this instance it doesn't actually break anything.
Speaking more generally if there were tests or other dependent code that broke as a result of some change it also depends, maybe the tests would need fixing or updating, or we wouldn't make that change, or we would approach it completely differently. We approach it on a case by case basis.
We can certainly keep the mindset in future for cases where there might not be existing tests but question "what if there were" and consider what might break or how it might affect the approach we take. That sounds like a good idea to me. On the other hand in cases where there aren't really external dependencies or integrations in play I don't personally see tests themselves (or other internal APIs for example) as immutable.
There was a problem hiding this comment.
I'd rather we silently swallow the error here, because otherwise updating v2 -> v4 will cause MySQL to create an index that it immediately drops. Creating indexes is expensive (it's an O(N) operation, and a slow one), so the cost of doing this is significant, even if it's less architecturally pure to do so.
This PR removes the
nextrunindex on the jobs table, as per discussion in #106.