optimize direct timeline #7614

tateisu · 2018-05-24T19:58:02Z

The direct timeline is mix of DM from user and DM to user.
This PR separates query to pair of _from_me and _to_me, then merge after querying for performance.

Also this PR does:

Fix filter condition. old implementation use filters for public timeline, but this TL is NOT public.
Fix bug in rspec test.
Optimize pagination of _to_me part that using mentions.status_id instead of statuses.id
Prevent conversion of constant condition into bind parameter. this is required to use partial index when prepared statement is enabled.

Temporary revoked:

Add partial indexes.
Add mentions.direct column that is used in partial index.

some information that is NOT included in this PR.

using union for merge 2 query on DB side
https://gist.github.com/tateisu/a7de7a8ff816e83ea2894e599499849d

too hard to pagination.

adding partial index for statuses
https://gist.github.com/tateisu/cc6bff006b898094262245491b631f2f

this query still scan all DM and then filter it.

akihikodaki · 2018-05-25T01:02:43Z

Thank you for your effort to address the issue.

Query and index optimization are for different purposes, and if you see improvements in both, probably you would need index optimization too. In case of your query, _from_me would take much if an account has few direct statuses without an index.

I tested an index after some discussion with you on Mastodon. In case of @173210@o.kagucho.net, though, an index greatly contributed to the performance and I do not see any room of improvement in the query plan.

Before applying an index:

mastodon=> explain analyze SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses"     
mastodon-> LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = 1    
mastodon-> LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
mastodon-> WHERE (mentions.account_id = 1 OR statuses.account_id = 1) AND "statuses"."visibility" = 3  
mastodon-> AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;
                                                                                       QUERY PLAN                                                     
                                  
------------------------------------------------------------------------------------------------------------------------------------------------------
----------------------------------
 Limit  (cost=18989.38..18989.38 rows=1 width=16) (actual time=6370.358..6370.369 rows=40 loops=1)
   ->  Sort  (cost=18989.38..18989.38 rows=1 width=16) (actual time=6363.888..6363.895 rows=40 loops=1)
         Sort Key: statuses.id DESC
         Sort Method: top-N heapsort  Memory: 26kB
         ->  Nested Loop  (cost=684.16..18989.37 rows=1 width=16) (actual time=357.594..6357.582 rows=123 loops=1)
               ->  Hash Left Join  (cost=683.87..18984.03 rows=1 width=24) (actual time=357.558..6353.799 rows=123 loops=1)
                     Hash Cond: (statuses.id = mentions.status_id)
                     Filter: ((mentions.account_id = 1) OR (statuses.account_id = 1))
                     Rows Removed by Filter: 7
                     ->  Index Only Scan using index_statuses_20180106 on statuses  (cost=0.42..18293.50 rows=145 width=24) (actual time=1.440..5994.2
96 rows=130 loops=1)
                           Index Cond: (visibility = 3)
                           Heap Fetches: 130
                     ->  Hash  (cost=680.05..680.05 rows=272 width=16) (actual time=356.077..356.077 rows=272 loops=1)
                           Buckets: 1024  Batches: 1  Memory Usage: 21kB
                           ->  Bitmap Heap Scan on mentions  (cost=10.53..680.05 rows=272 width=16) (actual time=13.166..355.843 rows=272 loops=1)
                                 Recheck Cond: (account_id = 1)
                                 Heap Blocks: exact=101
                                 ->  Bitmap Index Scan on index_mentions_on_account_id_and_status_id  (cost=0.00..10.46 rows=272 width=0) (actual time
=13.130..13.130 rows=272 loops=1)
                                       Index Cond: (account_id = 1)
               ->  Index Scan using accounts_pkey on accounts  (cost=0.29..5.32 rows=1 width=8) (actual time=0.029..0.029 rows=1 loops=123)
                     Index Cond: (id = statuses.account_id)
                     Filter: (NOT silenced)
 Planning time: 36.560 ms
 Execution time: 6373.108 ms
(24 rows)

Applying the index:

mastodon=> CREATE INDEX test ON statuses (account_id, id) WHERE visibility = 3;
CREATE INDEX

After applying the index and restarting PostgreSQL to invalidate caches on memory:

mastodon=> explain analyze SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses"     
LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = 1    
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
WHERE (mentions.account_id = 1 OR statuses.account_id = 1) AND "statuses"."visibility" = 3  
AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;
                                                                                    QUERY PLAN                                                        
                            
------------------------------------------------------------------------------------------------------------------------------------------------------
----------------------------
 Limit  (cost=1249.11..1249.12 rows=1 width=16) (actual time=763.299..763.306 rows=40 loops=1)
   ->  Sort  (cost=1249.11..1249.12 rows=1 width=16) (actual time=763.298..763.301 rows=40 loops=1)
         Sort Key: statuses.id DESC
         Sort Method: top-N heapsort  Memory: 26kB
         ->  Nested Loop  (cost=573.50..1249.10 rows=1 width=16) (actual time=603.562..763.148 rows=123 loops=1)
               ->  Hash Right Join  (cost=573.22..1243.77 rows=1 width=24) (actual time=586.983..688.652 rows=123 loops=1)
                     Hash Cond: (mentions.status_id = statuses.id)
                     Filter: ((mentions.account_id = 1) OR (statuses.account_id = 1))
                     Rows Removed by Filter: 7
                     ->  Bitmap Heap Scan on mentions  (cost=10.53..680.05 rows=272 width=16) (actual time=10.186..114.667 rows=272 loops=1)
                           Recheck Cond: (account_id = 1)
                           Heap Blocks: exact=101
                           ->  Bitmap Index Scan on index_mentions_on_account_id_and_status_id  (cost=0.00..10.46 rows=272 width=0) (actual time=10.15
6..10.156 rows=272 loops=1)
                                 Index Cond: (account_id = 1)
                     ->  Hash  (cost=560.87..560.87 rows=145 width=24) (actual time=573.690..573.690 rows=130 loops=1)
                           Buckets: 1024  Batches: 1  Memory Usage: 16kB
                           ->  Bitmap Heap Scan on statuses  (cost=8.83..560.87 rows=145 width=24) (actual time=3.466..573.501 rows=130 loops=1)
                                 Recheck Cond: (visibility = 3)
                                 Heap Blocks: exact=89
                                 ->  Bitmap Index Scan on test  (cost=0.00..8.79 rows=145 width=0) (actual time=0.055..0.055 rows=130 loops=1)
               ->  Index Scan using accounts_pkey on accounts  (cost=0.29..5.32 rows=1 width=8) (actual time=0.605..0.605 rows=1 loops=123)
                     Index Cond: (id = statuses.account_id)
                     Filter: (NOT silenced)
 Planning time: 393.201 ms
 Execution time: 763.777 ms
(25 rows)

Be aware that the account (and the database, too, as it is the only account on the instance) does not have that much direct statuses. It should use merge join instead of hash join for accounts with many direct statuses, but I'm not sure whether PostgreSQL planner is wise enough. The index should also be tested on accounts with many direct statuses. If not, this change would help PostgreSQL adopting a more optimal plan.

abcang · 2018-05-25T01:15:00Z

app/models/status.rb

@@ -196,6 +196,21 @@ def as_direct_timeline(account)
      apply_timeline_filters(query, account, false)
    end

+    def as_direct_timeline_from_me(account)
+      query = where("statuses.account_id = #{account.id}")


This is better

query = where(account_id: account.id)`

abcang · 2018-05-25T01:15:52Z

app/models/status.rb

+
+    def as_direct_timeline_to_me(account)
+      query = joins("LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = #{account.id}")
+              .where("mentions.account_id = #{account.id}")


Duplicate conditions of mentions.account_id

abcang · 2018-05-25T01:26:43Z

app/models/status.rb

+    end
+
+    def as_direct_timeline_to_me(account)
+      query = joins("LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id AND mentions.account_id = #{account.id}")


This is better

query = Status.joins(:mentions).merge(Mention.where(account_id: account.id))

abcang · 2018-05-25T01:28:32Z

app/controllers/api/v1/timelines/direct_controller.rb

+    (a1 + a2)
+      .sort { |a, b| b.id <=> a.id }
+      .uniq(&:id)
+      .slice(0, limit_param(DEFAULT_STATUSES_LIMIT))


Using take is simple.

.take(limit_param(DEFAULT_STATUSES_LIMIT))

abcang · 2018-05-25T01:32:54Z

app/controllers/api/v1/timelines/direct_controller.rb

+    a1 = cache_collection direct_statuses_from_me, Status
+    a2 = cache_collection direct_statuses_to_me, Status
+    (a1 + a2)
+      .sort { |a, b| b.id <=> a.id }


It is better to use sort_by.

.sort_by(&:id).reverse

abcang

as_direct_timeline is unnecessary and should be deleted.

tateisu · 2018-05-25T05:50:07Z

@abcang, all fixed.

@akihikodaki ,
Of course I agree that adding an index is necessary. However, it should be tested in several instances after this PR and then included in another PR.

I do not agree with "There is no room for improvement in the query plan." Merge Join is used in queries before applying this PR, but it Sorts the rows that is much larger than limit. There is no sort of mentions in the query after applying this PR. The number of unused rows is somewhat reduced.

It is not included in this PR, but if you devise more, you can add the pagenation condition not only for statuses.id but also for mentions.status_id . You may be able to reduce the unused rows in scanning mentions.

pagenation to statuses.id

SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses"     
LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
WHERE mentions.account_id = 1  AND "statuses"."visibility" = 3  
and statuses.id < 99722268340454164
AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;

Limit  (cost=0.99..1444.30 rows=1 width=16) (actual time=5.342..20.664 rows=40 loops=1)
  ->  Nested Loop  (cost=0.99..1444.30 rows=1 width=16) (actual time=5.332..20.362 rows=40 loops=1)
        ->  Nested Loop  (cost=0.70..1438.62 rows=1 width=24) (actual time=5.262..19.030 rows=40 loops=1)
              ->  Index Only Scan Backward using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..393.52 rows=161 width=8) (actual time=0.288..7.879 rows=724 loops=1)
                    Index Cond: (account_id = 1)
                    Heap Fetches: 629
              ->  Index Only Scan using statuses_dm on statuses  (cost=0.28..6.48 rows=1 width=24) (actual time=0.006..0.007 rows=0 loops=724)
                    Index Cond: ((id = mentions.status_id) AND (id < '99722268340454164'::bigint))
                    Heap Fetches: 24
        ->  Index Only Scan using accounts_not_silenced on accounts  (cost=0.29..5.67 rows=1 width=8) (actual time=0.014..0.018 rows=1 loops=40)
              Index Cond: (id = statuses.account_id)
              Heap Fetches: 38
Planning time: 1.906 ms
Execution time: 21.050 ms

pagenation to statuses.id and mentions.status_id

SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses"     
LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
WHERE mentions.account_id = 1  AND "statuses"."visibility" = 3  
and statuses.id < 99722268340454164 and mentions.status_id < 99722268340454164
AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;

Limit  (cost=0.99..1195.63 rows=1 width=16) (actual time=1.139..15.925 rows=40 loops=1)
  ->  Nested Loop  (cost=0.99..1195.63 rows=1 width=16) (actual time=1.130..15.627 rows=40 loops=1)
        ->  Nested Loop  (cost=0.70..1189.95 rows=1 width=24) (actual time=1.065..14.362 rows=40 loops=1)
              ->  Index Only Scan Backward using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..325.08 rows=134 width=8) (actual time=0.064..5.455 rows=564 loops=1)
                    Index Cond: ((account_id = 1) AND (status_id < '99722268340454164'::bigint))
                    Heap Fetches: 469
              ->  Index Only Scan using statuses_dm on statuses  (cost=0.28..6.44 rows=1 width=24) (actual time=0.007..0.007 rows=0 loops=564)
                    Index Cond: ((id = mentions.status_id) AND (id < '99722268340454164'::bigint))
                    Heap Fetches: 24
        ->  Index Only Scan using accounts_not_silenced on accounts  (cost=0.29..5.67 rows=1 width=8) (actual time=0.012..0.016 rows=1 loops=40)
              Index Cond: (id = statuses.account_id)
              Heap Fetches: 38
Planning time: 1.478 ms
Execution time: 16.213 ms

Heap Fetches for mentions is decreased from 629 to 469.

akihikodaki · 2018-05-25T08:10:29Z

I do not agree with "There is no room for improvement in the query plan." Merge Join is used in queries before applying this PR, but it Sorts the rows that is much larger than limit. There is no sort of mentions in the query after applying this PR. The number of unused rows is somewhat reduced.

I guess you are talking about: https://gist.github.com/tateisu/cc6bff006b898094262245491b631f2f

QUERY PLAN                                                                                         
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 Limit  (cost=399.99..3372.87 rows=1 width=16) (actual time=21.193..25.817 rows=40 loops=1)
   ->  Nested Loop  (cost=399.99..3372.87 rows=1 width=16) (actual time=21.183..25.517 rows=40 loops=1)
         ->  Merge Left Join  (cost=399.70..3367.86 rows=1 width=24) (actual time=21.129..24.350 rows=40 loops=1)
               Merge Cond: (statuses.id = mentions.status_id)
               Filter: ((mentions.account_id = 1) OR (statuses.account_id = 1))
               Rows Removed by Filter: 48
               ->  Index Only Scan Backward using statuses_dm on statuses  (cost=0.28..2962.95 rows=1869 width=24) (actual time=0.389..2.157 rows=88 loops=1)
                     Heap Fetches: 88
               ->  Sort  (cost=399.42..399.82 rows=161 width=16) (actual time=20.504..21.093 rows=148 loops=1)
                     Sort Key: mentions.status_id DESC
                     Sort Method: quicksort  Memory: 117kB
                     ->  Index Only Scan using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..393.52 rows=161 width=16) (actual time=0.055..13.303 rows=1470 loops=1)
                           Index Cond: (account_id = 1)
                           Heap Fetches: 1182
         ->  Index Only Scan using accounts_not_silenced on accounts  (cost=0.29..5.00 rows=1 width=8) (actual time=0.010..0.014 rows=1 loops=40)
               Index Cond: (id = statuses.account_id)
               Heap Fetches: 39
 Planning time: 21.167 ms
 Execution time: 26.263 ms
(19 rows)

Here you have to pay attention to two parts, which have different reasons why they query so many statuses:

               ->  Index Only Scan Backward using statuses_dm on statuses  (cost=0.28..2962.95 rows=1869 width=24) (actual time=0.389..2.157 rows=88 loops=1)
                     Heap Fetches: 88

               ->  Sort  (cost=399.42..399.82 rows=161 width=16) (actual time=20.504..21.093 rows=148 loops=1)
                     Sort Key: mentions.status_id DESC
                     Sort Method: quicksort  Memory: 117kB
                     ->  Index Only Scan using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..393.52 rows=161 width=16) (actual time=0.055..13.303 rows=1470 loops=1)
                           Index Cond: (account_id = 1)
                           Heap Fetches: 1182

The former one has tons of rows because it does not have Index Cond. That is because you cannot really have Index Cond because of the order of multicolumn index:

create index statuses_dm on statuses(id,account_id,updated_at) where visibility=3;

The problem is solved in my index.

I don't know about the later one, so it is concerning also for me. Ideally it should not have extra Sort and instead it should sort using index index_mentions_on_account_id_and_status_id. My result does not have the problem, but the table size may matter. The query using union does not, either. I'd like to know whether the queries introduced by this pull request do or do not.

tateisu · 2018-05-25T12:41:56Z

currently my instance has both of

statuses_dm on statuses (id, account_id, updated_at) WHERE visibility = 3;
statuses_dm_account on statuses(account_id, id, updated_at) WHERE visibility = 3;

and still statuses_dm is used for query that before apply this PR.
The selection of the query plan and the index may change depending on the data situation.

After applying this PR, _from_me part will be like as:

explain analyze SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses"     
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
WHERE statuses.account_id = 1 AND "statuses"."visibility" = 3  
and "statuses"."id" < 99722268340454164
AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;

 Limit  (cost=0.56..16.61 rows=1 width=16) (actual time=0.173..142.684 rows=40 loops=1)
   ->  Nested Loop  (cost=0.56..16.61 rows=1 width=16) (actual time=0.165..142.360 rows=40 loops=1)
         ->  Index Only Scan Backward using statuses_dm_account on statuses  (cost=0.28..8.29 rows=1 width=24) (actual time=0.111..140.563 rows=40 loops=1)
               Index Cond: ((account_id = 1) AND (id < '99722268340454164'::bigint))
               Heap Fetches: 31
         ->  Index Only Scan using accounts_not_silenced on accounts  (cost=0.29..8.31 rows=1 width=8) (actual time=0.020..0.025 rows=1 loops=40)
               Index Cond: (id = 1)
               Heap Fetches: 40
 Planning time: 0.819 ms
 Execution time: 143.018 ms

And _to_me part will be like as:

explain analyze SELECT "statuses"."id", "statuses"."updated_at" FROM "statuses"     
LEFT OUTER JOIN mentions ON statuses.id = mentions.status_id 
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
WHERE mentions.account_id = 1 AND "statuses"."visibility" = 3  
and "statuses"."id" < 99722268340454164
AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;

 Limit  (cost=0.99..1444.35 rows=1 width=16) (actual time=5.217..21.009 rows=40 loops=1)
   ->  Nested Loop  (cost=0.99..1444.35 rows=1 width=16) (actual time=5.207..20.702 rows=40 loops=1)
         ->  Nested Loop  (cost=0.70..1438.62 rows=1 width=24) (actual time=5.115..19.277 rows=40 loops=1)
               ->  Index Only Scan Backward using index_mentions_on_account_id_and_status_id on mentions  (cost=0.43..393.52 rows=161 width=8) (actual time=0.099..8.310 rows=724 loops=1)
                     Index Cond: (account_id = 1)
                     Heap Fetches: 629
               ->  Index Only Scan using statuses_dm on statuses  (cost=0.28..6.48 rows=1 width=24) (actual time=0.006..0.006 rows=0 loops=724)
                     Index Cond: ((id = mentions.status_id) AND (id < '99722268340454164'::bigint))
                     Heap Fetches: 24
         ->  Index Only Scan using accounts_not_silenced on accounts  (cost=0.29..5.72 rows=1 width=8) (actual time=0.014..0.018 rows=1 loops=40)
               Index Cond: (id = statuses.account_id)
               Heap Fetches: 38
 Planning time: 1.822 ms
 Execution time: 21.328 ms

The query plan becomes very simple and the query plan will be more stable.
This is the effect of this PR.

(wasteful reading of mentions is still exists, but this will not improve unless we change the table structure.)

Gargron · 2018-05-25T12:50:00Z

(wasteful reading of mentions is still exists, but this will not improve unless we change the table structure.)

How do you imagine the table structure needs to be changed?

Gargron · 2018-05-25T14:50:13Z

I wonder if there would be a benefit in storing mentioned account ids in an array column instead of separate table. (History: there was no "notifications" at the beginning, so instead there was a "mentions" column/API, so it made sense to have a separate table. Nowadays nothing queries the mentions table directly)

tateisu · 2018-05-25T15:31:09Z

to avoid waste reading of mentions table/index

(1) add visibility column to mentions.

alter table mentions add column visibility smallint not null default 0;
update mentions set visibility=statuses.visibility from statuses where statuses.id = mentions.status_id;
create index mentions_dm on mentions(account_id,status_id) where visibility=3;
(when insert record to mentions, set visibility column)
(this migration is very slow... update all rows in mentions table.)

or add direct column to mentions.

alter table mentions add column direct boolean not null default FALSE;
update mentions set direct=TRUE from statuses where statuses.id = mentions.status_id and statuses.visibility=3;
create index mentions_direct on mentions(account_id,status_id) where direct;
(when insert record to mentions, set direct column)
(this migration is faster, but supports only direct visibility)

(2) change pagenation to mentions.status_id instead of statuses.id

(3) change _to_me query like as:

explain analyze SELECT "statuses"."id", "statuses"."updated_at" 
FROM mentions
LEFT OUTER JOIN statuses ON statuses.id = mentions.status_id 
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id"       
WHERE mentions.account_id = 1 AND mentions.direct and statuses.visibility=3
and "mentions"."status_id" < 99722268340454164
AND "accounts"."silenced" = false ORDER BY "statuses"."id" DESC LIMIT 40;

then waste reading will be eliminated.

Limit  (cost=0.84..22.32 rows=1 width=16) (actual time=0.274..3.341 rows=40 loops=1)
   ->  Nested Loop  (cost=0.84..22.32 rows=1 width=16) (actual time=0.262..3.050 rows=40 loops=1)
         ->  Nested Loop  (cost=0.55..16.60 rows=1 width=24) (actual time=0.162..1.652 rows=40 loops=1)
               ->  Index Only Scan Backward using mentions_direct on mentions  (cost=0.28..8.29 rows=1 width=8) (actual time=0.085..0.235 rows=40 loops=1)
                     Index Cond: ((account_id = 1) AND (status_id < '99722268340454164'::bigint))
                     Heap Fetches: 0
               ->  Index Only Scan using statuses_dm on statuses  (cost=0.28..8.29 rows=1 width=24) (actual time=0.017..0.020 rows=1 loops=40)
                     Index Cond: (id = mentions.status_id)
                     Heap Fetches: 24
         ->  Index Only Scan using accounts_not_silenced on accounts  (cost=0.29..5.71 rows=1 width=8) (actual time=0.016..0.020 rows=1 loops=40)
               Index Cond: (id = statuses.account_id)
               Heap Fetches: 38
 Planning time: 3.212 ms
 Execution time: 3.665 ms

tateisu · 2018-05-25T15:47:01Z

@Gargron If the mentions table is structured to have an array data type of account for one status, it would be difficult to create an index equivalent to "index_mentions_on_account_id_and_status_id on mentions (account_id, status_id)".

tateisu · 2018-05-25T18:32:34Z

add some partial index
add mentions.direct column that is used for partial index.
migration updates mentions.direct column for each DIRECT status.
mentions.before_save callback is used to update mentions.direct column before INSERT/UPDATE
Status.as_direct_timeline_to_me(account) is changed to use partial index of mentions.
Api::V1::Timelines::DirectController.direct_statuses_to_me is changed to pagenate for mentions.status_id instead of statuses.id

I think that all my ideas were implemented. Please let us know if you have any comments.

tateisu · 2018-05-25T18:38:54Z

after applied this RP, this query is used to get statuses that mentions to the user.

SELECT  "statuses"."id", "statuses"."updated_at" FROM "statuses" 
INNER JOIN mentions on statuses.id = mentions.status_id 
LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id" 
WHERE (mentions.direct and mentions.account_id= 1) AND "statuses"."visibility" = 3 AND 1=1 
AND "accounts"."silenced" = FALSE ORDER BY "statuses"."id" DESC, mentions.status_id desc LIMIT 20

Limit  (cost=0.55..30.51 rows=1 width=24) (actual time=0.041..0.079 rows=7 loops=1)
  ->  Nested Loop  (cost=0.55..30.51 rows=1 width=24) (actual time=0.041..0.078 rows=7 loops=1)
        ->  Nested Loop  (cost=0.27..28.67 rows=1 width=32) (actual time=0.027..0.054 rows=7 loops=1)
              ->  Index Only Scan Backward using index_mentiond_direct on mentions  (cost=0.13..8.19 rows=3 width=8) (actual time=0.021..0.028 rows=7 loops=1)
                    Index Cond: (account_id = 1)
                    Heap Fetches: 7
              ->  Index Scan using index_statuses_dm on statuses  (cost=0.14..6.82 rows=1 width=24) (actual time=0.003..0.003 rows=1 loops=7)
                    Index Cond: (id = mentions.status_id)
        ->  Index Only Scan using index_accounts_not_silenced on accounts  (cost=0.28..1.82 rows=1 width=8) (actual time=0.002..0.003 rows=1 loops=7)
              Index Cond: (id = statuses.account_id)
              Heap Fetches: 7
Planning time: 5.516 ms
Execution time: 0.177 ms

tateisu · 2018-05-25T23:57:32Z

I've check query log by customizing PostgreSQL's log_statement.

the constant parameter such as "where visibility=3" is changed to bind parameter for prepared statements by Rails's default behavior.

then PostgreSQL can't use partial index because that decides which index is used when statement is prepared, this timing PostgreSQL can't know about bind parameter.

to avoid this, set PREPARED_STATEMENTS=false in .env.production.
If your instance already use pgbouncer, maybe already it is set.

tateisu · 2018-05-26T01:56:50Z

Now using "arel_table" to disable "bind parameter for constant condition".
This is required for using partial index.
Works fine if even PREPARED_STATEMENTS=false is NOT set (default).

query log

LOG:  execute <unnamed>: SELECT  "statuses"."id", "statuses"."updated_at" FROM "statuses" 
    LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id" 
    WHERE "statuses"."account_id" = $1 AND "statuses"."visibility" = 3 
    AND "statuses"."account_id" != $2 AND "accounts"."silenced" != TRUE 
    ORDER BY "statuses"."id" DESC LIMIT $3
DETAIL:  parameters: $1 = '1', $2 = '732', $3 = '20'

LOG:  execute <unnamed>: SELECT  "statuses"."id", "statuses"."updated_at" FROM "statuses" 
    INNER JOIN "mentions" ON "mentions"."status_id" = "statuses"."id" 
    LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id" 
    WHERE "mentions"."account_id" = $1 AND "mentions"."direct" = TRUE AND "statuses"."visibility" = 3 
    AND "statuses"."account_id" != $2 AND "accounts"."silenced" != TRUE 
    ORDER BY "statuses"."id" DESC, mentions.status_id desc LIMIT $3
DETAIL:  parameters: $1 = '1', $2 = '732', $3 = '20'

constant conditions (direct,silenced,visibility) is not parameterized.

tateisu · 2018-05-26T14:45:16Z

Fixed all things suggested at Mastodon from @abcang .
Please review this PR.

tateisu · 2018-05-26T16:07:49Z

commits rebased.

tateisu · 2018-05-27T04:27:40Z

index size example:

statuses 2.71GB
index_statuses_dm_account 56KB
index_statuses_dm 48KB

mentions 183MB (after adding "direct" column )
index_mentions_direct 48KB

accounts 111MB
index_accounts_not_silenced 1.2MB

tateisu · 2018-05-27T06:22:49Z

I did not intend to increase this PR at the first stage.
However, the comment received was a question about indexes and additional columns.
I enlarged PR to present implementation examples, but if there is an opinion that PR should be made small, it is possible to divide PR into several stages.

takayamaki · 2018-05-27T06:41:41Z

I feel that This PR is too big and Premature optimization.

My PR #7633 is about x100 faster than before the commit.
Also, #7614 before adding some commit was the same.
I think that it is fast enough.

At first, we should merge without changing tables and release as rc.
Then if still too slow in production, we may consider adding partial index or column at that time.

tateisu · 2018-05-27T07:01:55Z

Even if I divide the PR, I think that I keep the following items.
"Optimize pagination of _to_me part that using mentions.status_id instead of statuses.id"
It will make difference in case of benchmark with pagination.

tateisu · 2018-05-27T07:04:10Z

About this case, I will wait for the judgment by project collaborators.

tateisu · 2018-05-27T15:53:20Z

I thought about it now.

Direct message TL is close to home + notification TL rather than public TL.
But current implementation just calls apply_timeline_filters as well as public timeline.
There is no need to filter silence in this TL?
I should be able to read toots _from_me and _to_me even if accounts is silenced.

(_from_me) any filters are unnecessary.
(_to_me) not_excluded_by_account: may we check mutes.hide_notifications?
(_to_me) account_silencing_filter: certainly unnecessary!
(_to_me) not_in_filtered_languages: unnecessary? this is used for public TL.
(_to_me) not_domain_blocked_by_account: unnecessary? this is used for public TL.

please comment about this.

…se it.

tateisu · 2018-05-27T19:11:19Z

change filter condition for direct timeline. old implementation use filters for public timeline, but direct timeline is not public.
revoke introducing index_accounts_not_silenced because it is not used for direct timeline.
fix rspec test : re-querying TL after append mentions fabric.

tateisu · 2018-05-27T19:36:37Z

in commit be1d38a and e66b427, I've temporary revoke additional column and partial index.
if you have try previous commits, please db:rollback sometimes BEFORE merge this commit.

migration IDs are:

20180526013700
20180526013701
20180526013702
20180526013703

I believe there is no problems in additional column and partial index for performance and disk space, but some users hate it.

Gargron · 2018-05-27T22:21:59Z

app/controllers/api/v1/timelines/direct_controller.rb

-    cache_collection direct_statuses, Status
+    a1 = cache_collection direct_statuses_from_me, Status
+    a2 = cache_collection direct_statuses_to_me, Status
+    (a1 + a2).uniq(&:id).sort_by(&:id).reverse.take(limit_param(DEFAULT_STATUSES_LIMIT))


The question is, will the two collections ever be used separately? If not, it doesn't make sense to put this logic into a controller, and should rather become either a model method or a lib finder class. I also see no need to cache_collection them separately, since cache_collection takes (id, updated_at) objects and turns them into full statuses, you could use that on the end result array, e.g. keep cache_collection in the controller while the business logic is somewhere else.

I've fix it.

akihikodaki · 2018-05-28T01:23:09Z

currently my instance has both of

statuses_dm on statuses (id, account_id, updated_at) WHERE visibility = 3;

statuses_dm_account on statuses(account_id, id, updated_at) WHERE visibility = 3;

and still statuses_dm is used for query that before apply this PR.

Sorry for late reply. It is natural, but I still don't think statuses_dm is necessary. The use illustrated in the result can be replaced with index statuses_pkey.

The query plan becomes very simple and the query plan will be more stable.

I'm not expecting his kind of query will be stable. The best plan varies by the scale of the table.

tateisu · 2018-05-28T04:30:43Z

@akihikodaki this PR currently does not contains index, but if I prepare the next PR, statuses_dm_account and mentions_dm will be included. after applied this PR, statuses_dm is used in _to_me part ,that checks visibility=3 and get updated_at with index only scan, but index only scan is not mandatory.

In case of DM TL, I recognize that the instance requiring query optimization is the majority. Your case is rather rare.

abcang · 2018-05-28T05:21:10Z

app/models/status.rb


-      apply_timeline_filters(query, account, false)
+      result = (query_from_me.to_a + query_to_me.to_a).uniq(&:id).sort_by(&:id).reverse.take(limit)
+      logger.debug result.inspect


logger.debug is left

tateisu · 2018-05-28T05:52:48Z

@Gargron
Some other method in Status returns ActiveRecord::Relation.
But in case of direct timeline, I can't keep it because query result merging is works on application side.
Status.as_direct_timeline in current commit returns array of Status or cache_ids object (selectable by method argument).

Is this acceptable?

tateisu · 2018-05-28T06:45:06Z

Now Status.as_direct_timeline returns ActiveRecord::Relation if cache_ids method argument is false.
( with using more 1 DB query. but currently it used only from spec test. )

Gargron · 2018-05-28T09:03:55Z

StatusThreadingConcern also does not return relation, that is fine

* optimize direct timeline * fix typo in class name * change filter condition for direct timeline * fix codestyle issue * revoke index_accounts_not_silenced because direct timeline does not use it. * revoke index_accounts_not_silenced because direct timeline does not use it. * fix rspec test condition. * fix rspec test condition. * fix rspec test condition. * revoke adding column and partial index * (direct timeline) move merging logic to model * fix pagination parameter * add method arguments that switches return array of status or cache_ids * fix order by * returns ActiveRecord.Relation in default behavor * fix codestyle issue

abcang reviewed May 25, 2018

View reviewed changes

ykzts added the performance Runtime performance label May 25, 2018

abcang approved these changes May 25, 2018

View reviewed changes

tateisu changed the title ~~optimize direct timeline~~ [WIP] optimize direct timeline May 25, 2018

tateisu changed the title ~~[WIP] optimize direct timeline~~ optimize direct timeline May 25, 2018

optimize direct timeline

7a6fc3b

tateisu force-pushed the tune-dm-timeline branch from d137a5b to 7a6fc3b Compare May 26, 2018 16:01

fix typo in class name

f87312b

takayamaki mentioned this pull request May 27, 2018

fix Status#as_direct_timeline querying #7633

Closed

tateisu added 5 commits May 28, 2018 03:42

revoke index_accounts_not_silenced because direct timeline does not u…

be1d38a

…se it.

revoke index_accounts_not_silenced because direct timeline does not u…

1cba54c

…se it.

fix rspec test condition.

bf37519

fix rspec test condition.

d805c59

fix rspec test condition.

c9e1bf5

revoke adding column and partial index

e66b427

tateisu force-pushed the tune-dm-timeline branch from 58efdd8 to e66b427 Compare May 27, 2018 19:39

Gargron requested changes May 27, 2018

View reviewed changes

abcang reviewed May 28, 2018

View reviewed changes

(direct timeline) move merging logic to model

bf4d7dc

tateisu force-pushed the tune-dm-timeline branch from 317e5ae to bf4d7dc Compare May 28, 2018 05:22

tateisu added 2 commits May 28, 2018 14:28

fix pagination parameter

fd2e41d

add method arguments that switches return array of status or cache_ids

7e49495

tateisu added 3 commits May 28, 2018 15:08

fix order by

d6ab541

returns ActiveRecord.Relation in default behavor

5a0f01f

fix codestyle issue

848b2df

Gargron approved these changes May 28, 2018

View reviewed changes

Gargron merged commit b87a122 into mastodon:master May 28, 2018

tateisu mentioned this pull request May 28, 2018

optimize direct message 2: add mentions.direct column and some partial index #7649

Closed

optimize direct timeline #7614

optimize direct timeline #7614

Conversation

tateisu commented May 24, 2018 • edited Loading

akihikodaki commented May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abcang May 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abcang left a comment

Choose a reason for hiding this comment

tateisu commented May 25, 2018

akihikodaki commented May 25, 2018

tateisu commented May 25, 2018 • edited Loading

Gargron commented May 25, 2018

Gargron commented May 25, 2018

tateisu commented May 25, 2018 • edited Loading

tateisu commented May 25, 2018

tateisu commented May 25, 2018

tateisu commented May 25, 2018

tateisu commented May 25, 2018 • edited Loading

tateisu commented May 26, 2018 • edited Loading

tateisu commented May 26, 2018

tateisu commented May 26, 2018

tateisu commented May 27, 2018

tateisu commented May 27, 2018

takayamaki commented May 27, 2018

tateisu commented May 27, 2018

tateisu commented May 27, 2018

tateisu commented May 27, 2018 • edited Loading

tateisu commented May 27, 2018

tateisu commented May 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akihikodaki commented May 28, 2018

tateisu commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tateisu commented May 28, 2018 • edited Loading

tateisu commented May 28, 2018

Gargron commented May 28, 2018

tateisu commented May 24, 2018 •

edited

Loading

akihikodaki commented May 25, 2018 •

edited

Loading

abcang May 25, 2018 •

edited

Loading

tateisu commented May 25, 2018 •

edited

Loading

tateisu commented May 25, 2018 •

edited

Loading

tateisu commented May 25, 2018 •

edited

Loading

tateisu commented May 26, 2018 •

edited

Loading

tateisu commented May 27, 2018 •

edited

Loading

tateisu commented May 27, 2018 •

edited

Loading

tateisu commented May 28, 2018 •

edited

Loading