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 a partial index on account move line #7430

Closed
wants to merge 1 commit into from

Conversation

guewen
Copy link
Contributor

@guewen guewen commented Jul 2, 2015

A log analysis showed that the normalized query below was executed very often
with a slow explain plan using a seq scan.

SELECT move_id, date
FROM account_move_line
WHERE journal_id = 0
AND period_id = 0
AND create_uid = 0
AND state = ''
ORDER BY id DESC LIMIT 0;

This query is called in the _default_get of account_move_line to find the last
unbalanced move line. The state is always 'draft' and the fields always
journal_id, period_id and create_uid so it is a good candidate for a partial
index with combined fields.

The explain plan shows an impressive improvement, dropping from ~800ms to
~0.030 ms for a table with 3906500 records. This query was called 7700 times in
a working day for a cumulated duration of 25m32s.

Example of explain plan before:

# explain analyze SELECT move_id, date FROM account_move_line WHERE journal_id = 2 AND period_id = 60 AND create_uid = 1 AND state = 'draft' ORDER BY id DESC LIMIT 1;
                                                                                 QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=86399.22..86399.22 rows=1 width=12) (actual time=852.562..852.562 rows=0 loops=1)
   ->  Sort  (cost=86399.22..86399.22 rows=1 width=12) (actual time=852.560..852.560 rows=0 loops=1)
         Sort Key: id
         Sort Method: quicksort  Memory: 25kB
         ->  Bitmap Heap Scan on account_move_line  (cost=1189.59..86399.21 rows=1 width=12) (actual time=852.555..852.555 rows=0 loops=1)
               Recheck Cond: ((journal_id = 2) AND (period_id = 60))
               Filter: ((create_uid = 1) AND ((state)::text = 'draft'::text))
               Rows Removed by Filter: 50824
               Heap Blocks: exact=9317
               ->  Bitmap Index Scan on account_move_line_journal_id_period_id_index  (cost=0.00..1189.59 rows=56516 width=0) (actual time=8.983..8.983 rows=50824 loops=1)
                     Index Cond: ((journal_id = 2) AND (period_id = 60))
 Planning time: 0.702 ms
 Execution time: 852.592 ms

And after:

# explain analyze SELECT move_id, date FROM account_move_line WHERE journal_id = 2 AND period_id = 60 AND create_uid = 1 AND state = 'draft' ORDER BY id DESC LIMIT 1;
                                                                                          QUERY PLAN
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=8.08..8.09 rows=1 width=12) (actual time=0.012..0.012 rows=0 loops=1)
   ->  Sort  (cost=8.08..8.09 rows=1 width=12) (actual time=0.011..0.011 rows=0 loops=1)
         Sort Key: id
         Sort Method: quicksort  Memory: 25kB
         ->  Index Scan using account_move_line_journal_id_period_id_create_uid_state_index on account_move_line  (cost=0.14..8.07 rows=1 width=12) (actual time=0.007..0.007 rows=0 loops=1)
               Index Cond: ((journal_id = 2) AND (period_id = 60) AND (create_uid = 1))
 Planning time: 0.182 ms
 Execution time: 0.029 ms
(8 rows)

The query is called here: https://github.com/guewen/odoo/blob/7.0-account_move_line-join-index/addons/account/account_move_line.py#L268-L271

@pedrobaeza
Copy link
Collaborator

👍

It would be good to have a CREATE OR REPLACE option like in views.

@lmignon
Copy link
Contributor

lmignon commented Jul 2, 2015

👍

@nim-odoo nim-odoo self-assigned this Jul 6, 2015
@beledouxdenis beledouxdenis assigned odony and unassigned nim-odoo Jul 6, 2015
@odony
Copy link
Contributor

odony commented Jul 8, 2015

Thanks, improving the performance of this query is a good idea, but we should refrain from adding more indexes on that table, especially a partial one that would only be useful in a few limited cases. Instead we could improve an existing index (e.g. account_move_line_journal_id_period_id_index) to help for this specific query too.

If we extend it to include the state, create_uid and id DESC columns, resolving this default query can be done with a single INDEX SCAN without requiring any sort. And that index is useful for many other kinds of queries:

CREATE INDEX account_move_line_journal_id_period_id_index
ON account_move_line (journal_id, period_id, state, create_uid, id DESC)

This is the result of your benchmark:

# select count(*) from account_move_line;
  count  
---------
 3129408
(1 row)

# select count(*) from account_move_line where journal_id = 29 AND period_id = 12;
  count  
---------
 2875396
(1 row)

# explain analyze SELECT move_id, date FROM account_move_line WHERE journal_id = 29 AND period_id = 12 AND create_uid = 1 AND state = 'draft' ORDER BY id DESC LIMIT 1;
                                                                                    QUERY PLAN                                                                                    
----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=0.00..8.90 rows=1 width=12) (actual time=0.107..0.107 rows=0 loops=1)
   ->  Index Scan using account_move_line_journal_id_period_id_uid_state_id_idx on account_move_line  (cost=0.00..8.90 rows=1 width=12) (actual time=0.104..0.104 rows=0 loops=1)
         Index Cond: ((journal_id = 29) AND (period_id = 12) AND ((state)::text = 'draft'::text) AND (create_uid = 1))
 Total runtime: 0.174 ms
(4 rows)

Changing this in 7.0 is a bit borderline, and the performance boost will not affect many operations, but if it only involves adding a few extra columns in the existing index I think it is acceptable. Users who would like to benefit from the improvement will have to drop (and recreate) the index manually, which is safer.

What do you think?

An log analysis showed that the normalized query below was executed very often
with a slow explain plan using a seq scan.

  SELECT move_id, date
  FROM account_move_line
  WHERE journal_id = 0
  AND period_id = 0
  AND create_uid = 0
  AND state = ''
  ORDER BY id DESC LIMIT 0;

This query is called in the _default_get of account_move_line to find the last
unbalanced move line.

The existing index can be improved to cover this query as well, showing a
Impressive improvement of the explain plan as explained here:
odoo#7430 (comment)
@guewen guewen force-pushed the 7.0-account_move_line-join-index branch from cfc7eb7 to 555a595 Compare July 8, 2015 10:45
@guewen
Copy link
Contributor Author

guewen commented Jul 8, 2015

Thanks a lot @odony for this very detailed analysis and feedback. I'm always delighted when I get a review from you because I know that I will learn and that you will raise the good questions or improvements. ⭐

I updated my commit according to your comment.

odony pushed a commit that referenced this pull request Jul 8, 2015
….line

A log analysis showed that the normalized query below was executed very often
with a slow explain plan using a seq scan.

```sql

SELECT move_id, date
FROM account_move_line
WHERE journal_id = <journal_id>
AND period_id = <period_id>
AND create_uid = <user_id>
AND state = 'draft'
ORDER BY id DESC LIMIT 0;

```

This query is called in the _default_get of account.move.line to find the last
unbalanced move line.

The existing index can be improved to cover this query as well, showing an
impressive improvement of the explain plan as explained here:
#7430 (comment)

Closes #7430
@odony
Copy link
Contributor

odony commented Jul 8, 2015

Thanks for the quick update of the PR, just merged in 7.0 at 4fe0c6b :-)

@odony odony closed this Jul 8, 2015
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.

None yet

5 participants