Move to MySQL8 #5364
Labels
Comments
|
Just an update: I figured out the issue with MySQL8 tests. It was a query optimizer that was over eager. |
bors bot
added a commit
that referenced
this issue
Feb 22, 2021
5372: Rework stock_movement_status calculation r=jmcameron a=jniles This is a full rewrite of the `stock_movement_status` table and associated stored procedures. The goal of this rewrite is to improve clarity, remove user-defined variables that may potentially collide across MySQL connections, and improve the performance of the `GetCMM()` calculation. Closes #5073. Closes #5144. Closes #5332. Closes #5360. ----    This PR contains many far reaching changes. Here is a lot of the notable ones: 1. I've removed the `avg_consumption` column from the inventory table. It was never used, as far as I can tell, to do any reporting. It was used in the stock import feature, but we don't use the `avg_consumption` to report on, so I just removed that code. 2. I've removed the `stock_consumption` table and associated procedures. It is used in several reports and they are now broken. However, I would argue they were always broken since the `stock_consumption` did not record accurate information about stock consumption. 3. I've entirely changed the structure behind the `stock_movement_status` table for the following reasons: 1. The [previous structure](https://github.com/IMA-WorldHealth/bhima/blob/e455a8011db69ad48839afc35977d3dd041d3241/server/models/schema.sql#L2016) was very hard to audit. For example, it had `start_date`, `end_date`, and `quantity`. Is that the quantity at the start? The quantity at the end? If I wanted to find the value on date X, I needed to do a BETWEEN query. 2. The table did not distinguish between consumption information and exit information. This made the `GetCMM()` calculation have to look up additional information from the `stock_movement` table. A user would need to as well. 3. In theory, we'll be calling `GetCMM()` more than we will be moving stock. It makes sense to make the performance tradeoff to increase the speed of `GetCMM()` and decrease the speed of inserting stock. 5. I've renamed the `GetCMM()` call to `GetAMC()` for clarity. It also takes in a single date and uses the depot to find the correct value of the date range. This prevents us from having inconsistent results because a developer didn't properly compute the date range. If we need to support a custom date range, we'll just write another SP. 6. I've slightly changed the API of `GetAMC()`: 1. It returns `head_days` and `tail_days` instead of `days_before_stock_consumption`. That is because `days_before_stock_consumption` is the real number of days before the first consumption. Instead, it the days between the start of the window and the next record. Similarly, `tail_days` gives you the number of days between the last consumption record and the end of the window. 2. It returns the `quantity_in_stock` so that we can perform stock out date calculations. 3. `first_inventory_movement_date` -> `min_date` 4. `last_inventory_movement_date` -> `max_date` ------ It isn't all good. In fact, initializing this is terrible. On my machine (4GB of RAM): ```mysql mysql> use vanga; mysql> CALL zRecomputeStockMovementStatus(); Query OK, 0 rows affected (22 min 4.81 sec) mysql> use imck; mysql> CALL zRecomputeStockMovementStatus(); Query OK, 0 rows affected (4 min 20.31 sec) ``` This is _horrible_. I did get a lot faster speeds when I was just testing on MySQL 8, but the syntax didn't work on MySQL 5.7 (#5364). When I fixed it on MySQL5.7, I ran into issues on MySQL 8. Basically, we got the worst performance of both worlds by trying to get it to work correctly on both worlds. However, I still think it is worth merging because the performance of any individual transaction isn't bad. It is only when rebuilding the database that things go horribly long. ---- ### How to test Download this PR and run the tests! (`yarn test:integration` and `yarn test:integration:stock`). If that works, try with a production database. Note, as above, you'll need to `CALL zRecomputeStockMovementStatus()` to build the table. Co-authored-by: Jonathan Cameron <jmcameron@gmail.com> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com> Co-authored-by: mbayopanda <mbayopanda@gmail.com> Co-authored-by: Jonathan Niles <jonathanwniles@gmail.com>
jniles
added a commit
to jniles/bhima
that referenced
this issue
Jun 15, 2021
Following the decision of the team, we've dropped support for all older version of MySQL than 8. Closes Third-Culture-Software#5364.
bors bot
added a commit
that referenced
this issue
Jun 15, 2021
5756: Chore: remove testing on MySQL 5.7 r=jniles a=jniles This commit removes testing on MySQL 5.7 and also removes the documentation tied to MySQL 5.7. It completes the transition to MySQL 8. Closes #5364. ----- [View rendered docs/en/for-developers/installing-bhima.md](https://github.com/jniles/bhima/blob/chore-remove-testing-on-mysql-5.7/docs/en/for-developers/installing-bhima.md) [View rendered docs/fr/for-developers/installing-bhima.md](https://github.com/jniles/bhima/blob/chore-remove-testing-on-mysql-5.7/docs/fr/for-developers/installing-bhima.md) Co-authored-by: Jonathan Niles <jonathanwniles@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
For some of our advanced queries, it would b really helpful to move entirely to MySQL 8. Specifically, it would be nice to have window functions and partitioning.
We have a few places where we hack this together for the "simpler" queries for running balance. However, if we want to calculate multiple running balances (as we do in the CMM calculations), we'll want to keep track of multiple running balances at a time. I can't figure out an easy way to do this in MySQL 5.7, but it is trivial in MySQL 8:
These benefits (plus the utf8mb4 by default, etc) should make us take a serious look at moving entirely to MySQL 8.
We do have issues in our trial balance tests for MySQL 8 that should give us pause ... but in theory it's a misunderstanding on our part and we can figure them out.UPDATE: solved this.The text was updated successfully, but these errors were encountered: