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

[ADD] pabi_direct_sql, improve performance #1817

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

kittiu
Copy link
Collaborator

@kittiu kittiu commented Apr 6, 2020

By using direct SQL on some key models.
From initial test, to "Confirm Oder" of a draft purchase order with 10 lines, speed up from 60 secs to 5 secs
But this should boost performance of overall budget commitment and actuals.
Note: This will need a lot of test, as we use sql instead of orm.

For this PR, it is for create() method of account_analytic_line, of which logics includes,

  • All logic of [account.analytic.line].create()
  • 4 ORM fileds, _prepare_orm_defaults
  • Defaults, _prepare_defaults
  • Related fields, _prepare_related_values
  • Compute fields (with store=True)
  • Constraints (that update fields)

To use this, please add system param pabi_direct_sql.analytic_create = True

@kittiu kittiu force-pushed the kittiu-add-pabi_direct_sql-performance_tuning branch 2 times, most recently from 2505440 to 56d3520 Compare April 6, 2020 07:10
@kittiu
Copy link
Collaborator Author

kittiu commented Apr 6, 2020

With ORM

orm

With Direct SQL

direct_sql

related = {
# From account.account_analytic_line.py
'currency_id': ('move_id', 'currency_id', 'account_move_line'),
'account_currency': ('move_id', 'amount_currency',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
'account_currency': ('move_id', 'amount_currency',
'amount_currency': ('move_id', 'amount_currency',

@kittiu kittiu force-pushed the kittiu-add-pabi_direct_sql-performance_tuning branch from 56d3520 to 13d633e Compare April 7, 2020 05:16
@kittiu
Copy link
Collaborator Author

kittiu commented Apr 7, 2020

Copy link
Collaborator

@Saran440 Saran440 left a comment

Choose a reason for hiding this comment

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

In database there are some fields disappear when not value
i.e. amount_currency, has_commit_amount, chart_view, require_chartfield
Selection_010


@api.model
def _prepare_orm_defaults(self, vals):
today = vals.get('write_date', fields.Date.context_today(self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
today = vals.get('write_date', fields.Date.context_today(self))
today = vals.get('write_date', fields.Datetime.now())

In database keep datetime and
Should be used format 0.## in code before create analytic line?
Selection_008

@kittiu
Copy link
Collaborator Author

kittiu commented Apr 7, 2020

@Saran440 thanks for your review. Can you ping me when you finish the review? I will do it once.

@Saran440
Copy link
Collaborator

Saran440 commented Apr 7, 2020

Can't Create IN Transfer
Selection_001

Step to error:

  1. duplicate PO (In Deliveries & Invoices Tab select Invoicing Control = Based in Purchase Order lines and Payment Term = N180)
    Selection_002
  2. Confirm PO -> Release -> Create Work Acceptance (Receive 1 and state draft)
  3. Receive Product -> add WA -> Transfer

Error in function _prepare_compute_document line document = move.document_id

@Saran440
Copy link
Collaborator

Saran440 commented Apr 7, 2020

@kittiu Reviewed done. but I have a question.
Why is module account.analytic.line keep amount from amount_currency?
i.e. Create 1 product 10 usd (300 Baht) it should be amount = 300, amount_currency = 10
but this case is amount = 10, amount_currency = False

@kittiu
Copy link
Collaborator Author

kittiu commented Apr 7, 2020

Thanks for the review, it was very helpful. Many things that I have to fix.
So, please test more :) it is better to fix now than later.

@kittiu kittiu force-pushed the kittiu-add-pabi_direct_sql-performance_tuning branch from edeea9a to 5544c70 Compare April 7, 2020 16:06
@kittiu
Copy link
Collaborator Author

kittiu commented Apr 7, 2020

@kittiu Reviewed done. but I have a question.
Why is module account.analytic.line keep amount from amount_currency?
i.e. Create 1 product 10 usd (300 Baht) it should be amount = 300, amount_currency = 10
but this case is amount = 10, amount_currency = False

I tested with supplier invoice, using USD for $1, and it works fine. amount = 30 and amount_currency = 1

@kittiu
Copy link
Collaborator Author

kittiu commented Apr 7, 2020

All fixed, please try again.

Note: only the decimal still .0 for some tech reason, but if there more digits, i.e., 1.1232939, I can round it to 1.12

@Saran440
Copy link
Collaborator

Saran440 commented Apr 8, 2020

@kittiu

Functional Test

Purchase Order

  • PO -> WA -> IN
    Create IN Transfer but some fields disappear
    Selection_005

  • when Confirm PO (used pabi_direct_sql.analytic_create) has_commit_amount = False.
    Not used pabi_direct_sql.analytic_create has_commit_amount = True
    Selection_006

Expense

  • Interface EX -> accept -> Create Supplier Invoice -> Validate (invoice)
    Selection_009

Compare line amount negative (ID 2371845, 2371849)
Selection_008

  • there are some fields (document, document_id). It shouldn't have.
  • doctype should be adjustment
  • create_date and write_date are not equal (a little bit)

Compare line amount positive (ID 2371846, 2371850)
Selection_010

  • period_id and quarter are not equal

@kittiu
Copy link
Collaborator Author

kittiu commented Apr 8, 2020

@Saran440 good news, I found that the main problem of performance issue is just this file,
https://github.com/pabi2/pb2_addons/pull/1817/files#diff-a0a19dff3eed59e031c0996bff63c94a
You can deploy this file alone and get 10x performance on PO confirm without worry of the direct sql.

@kittiu
Copy link
Collaborator Author

kittiu commented Apr 9, 2020

แก้จบแล้ว ยกเว้น

* there are some fields (document, document_id). It shouldn't have.
* doctype should be adjustment

doctype = adjustment, I think the old system was wrong. But may be no lone notice because no report against it.
For the old and new quarter, I also think all should be Q3. But not very sure.

In summary, considered these are fixed so far.

@kittiu
Copy link
Collaborator Author

kittiu commented Apr 9, 2020

Let's pause this for now, let's try to fix problem by index only first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants