Skip to content
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

bug(Stock Value report crashes server with too much data) #4556

Merged

Conversation

lomamech
Copy link
Collaborator

  • Improvement and optimization of the stock value by deposit report

closes #4116

@lomamech
Copy link
Collaborator Author

@jniles @mbayopanda I managed to optimize the report, please do the review

Copy link
Collaborator

@jniles jniles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to test this, but it still crashes for me. My virtual machine only has 4GB of RAM, but that should be more than enough. After 2 minutes, the request times out and retries. It also pegs my MySQL CPU usage at over 300%.

image

I'm not sure this solves the problem at all. I still think, as mentioned in our previous discussion, that something like period_total is the way to go. That way, we are doing very little computation in the actual report itself.

caKfGd60mv

SET character_set_database = 'utf8mb4';
SET collation_database = 'utf8mb4_unicode_ci';
SET CHARACTER SET utf8mb4, CHARACTER_SET_CONNECTION = utf8mb4;
SET collation_connection = 'utf8mb4_unicode_ci';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? The SQL dump should be exporting with these defaults. Is that not the case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary? The SQL dump should be exporting with these defaults. Is that not the case?

This portion of code was added when I rebase with master, I haven't added it personally

* but also the cumulative value of entries *quantityCum* and exits *cumOut*
* and the quantity in stock after each movement of stock *quantityStock*
*/
const stockValues = await db.exec(sqlGetInventories, [db.bid(options.depot_uuid), options.dateTo]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sp, if I am understanding this correctly, this code will generate 1 + N SQL queries in a transaction, where N is the number of inventory items in a depot. At Vanga, the total number of inventory items is currently 784. At IMCK, the total number is 1302. So we could potentially get hundreds of SQL queries launched from a single query here.

... I'm not sure how optimal this is in low-end hardware like a Raspberry Pi.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the way I understood the logic of the stock value report came down to retrieving the last line of the stock card report, my idea was to launch several requests on the stock movement table in order to calculate for each unit cost weight and the stock value after each transaction

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had given up on the idea of using an aggregation table because of the risk of having backdated stock movements

});
});

let stockTolal = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a typo - it should be "stockTotal"

<tr>
<td style="width:50%">{{inventory_name}}</td>
<td class="text-right">{{stockQtt}}</td>
<td class="text-right">{{currency (multiply stockUnitCost ../rate) ../currency_id}}</td>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do this multiplication before putting it in the template?

</tr>
{{/each}}
<!-- no data -->
{{#if emptyResult}}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no need for this "emptyResult" check. Just use {{#each}}{{else}}{{/each}} as discussed in WhatsApp.

So, it would look like this:

{{#each stockValues}}
   {{! ... etc ... }}
{{else}}
  <tr><th class="text-center" colspan="4">{{translate 'STOCK.NO_DATA'}}</th></tr>
{{/each}}

const options = (typeof (_options.params) === 'string') ? JSON.parse(_options.params) : _options.params;
data.dateTo = options.dateTo;
data.depot = await db.one('SELECT * FROM depot WHERE uuid=?', [db.bid(options.depot_uuid)]);
const stockValues = await db.exec('CALL stockValue(?,?,?);', [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not longer using the stockValue() procedure, let's remove it from the stored procedures.


const stockValueElements = options.exclude_zero_value
? stockValues[0].filter(item => item.stockValue > 0) : stockValues[0];
? stockValues.filter(item => item.stockValue > 0) : stockValues;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anyway we can apply this filter higher in logic? Preferably in the SQL. It would probably improve performance.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it will be difficult seeing that the products we see the stock shortage of a product after on the last line, after having noted all the entries and exits from Stock

@lomamech
Copy link
Collaborator Author

I tried to test this, but it still crashes for me. My virtual machine only has 4GB of RAM, but that should be more than enough. After 2 minutes, the request times out and retries. It also pegs my MySQL CPU usage at over 300%.

image

I'm not sure this solves the problem at all. I still think, as mentioned in our previous discussion, that something like period_total is the way to go. That way, we are doing very little computation in the actual report itself.

caKfGd60mv

With me it works really well

m0p7jfOYIa

@jniles
Copy link
Collaborator

jniles commented May 28, 2020

That's because you have a machine with 16GB of RAM that cost many, many dollars. We can't expect poor hospitals to buy these kinds of computers.

@lomamech
Copy link
Collaborator Author

That's because you have a machine with 16GB of RAM that cost many, many dollars. We can't expect poor hospitals to buy these kinds of computers.

I think the problem stems from the way in which stock managers calculate the stock value of a product, I fear that even if we only had computers with large capacities, even after 10 or 15 years. use, that the stock value report then crashes out again

Copy link
Collaborator

@mbayopanda mbayopanda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

it seems like stock_sheet report is not available

@lomamech lomamech closed this Jun 1, 2020
lomamech added 5 commits June 2, 2020 06:29
- Improvement and optimization of the stock value by deposit report

closes Third-Culture-Software#4116
- Improvement and optimization of the stock value report, reduction of
  operations on the MYSQL side
@lomamech lomamech reopened this Jun 2, 2020
@lomamech lomamech force-pushed the fixRemakeStockValueReport branch from b1d7df6 to 299cb15 Compare June 2, 2020 13:02
@lomamech
Copy link
Collaborator Author

lomamech commented Jun 2, 2020

@jniles i need a review

Copy link
Collaborator

@jniles jniles left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much better for performance. The query takes 10 seconds on my machine (22 seconds total time). It also doesn't peg MySQL at 300% CPU usage. See below.

8wged7bEs0

I still think we can do better via period_total style table, but this fixes the bug in production to an acceptable level where the report is usable. Great work!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 3, 2020

Build succeeded:

@bors bors bot merged commit 4d04833 into Third-Culture-Software:master Jun 3, 2020
@lomamech lomamech deleted the fixRemakeStockValueReport branch October 2, 2020 07:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stock Value report crashes server with too much data
4 participants