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

Filter direct statuses in Status.as_home_timeline #3842

Merged
merged 1 commit into from Jun 20, 2017

Conversation

Projects
None yet
5 participants
@akihikodaki
Collaborator

akihikodaki commented Jun 18, 2017

The classes using Status.as_home_timeline, namely Feed and PrecomputeFeedService are expected to filter direct statuses as FanOutWriteService does, but their filtering were incomplete or missing.

This commit solves the problem by filtering direct statuses in as_home_timeline as the other similar methods such as as_public_timeline does.

Labelling priority - high; if my understanding is correct, the addressed bug had unexpectedly caused to make direct statuses visible for followers in the following certain condition:

  • They were offline for 24 hours (which forces to regenerate feed) AND
  • They reached to api/v1/timelines/home before the regenerations finishes.
@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 18, 2017

Collaborator

@alpaca-tc Could you test the following query? I'm not really confident with the performance of the query. This query change is related to #2967.

> EXPLAIN ANALYZE SELECT "statuses".* FROM "statuses" LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id" LEFT OUTER JOIN "follows" ON "follows"."target_account_id" = "accounts"."id" LEFT OUTER JOIN "accounts" "followers_accounts" ON "followers_accounts"."id" = "follows"."account_id" LEFT OUTER JOIN "mentions" ON "mentions"."status_id" = "statuses"."id" WHERE ((("statuses"."visibility" != 3) OR "mentions"."account_id" = 1) AND "follows"."account_id" = 1 OR "statuses"."account_id" = 1) GROUP BY "statuses"."id" ORDER BY "statuses"."id" DESC LIMIT 100;
                                                                                        QUERY PLAN                                                    
                                    
------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------
 Limit  (cost=0.00..380.01 rows=100 width=438) (actual time=17.460..18.884 rows=100 loops=1)
   ->  Group  (cost=0.00..155684.60 rows=40969 width=438) (actual time=17.459..18.870 rows=100 loops=1)
         ->  Nested Loop Left Join  (cost=0.00..155582.18 rows=40969 width=438) (actual time=17.456..18.767 rows=105 loops=1)
               Filter: ((((statuses.visibility <> 3) OR (mentions.account_id = 1)) AND (follows.account_id = 1)) OR (statuses.account_id = 1))
               Rows Removed by Filter: 45
               ->  Nested Loop Left Join  (cost=0.00..135363.65 rows=67145 width=442) (actual time=17.445..18.552 rows=139 loops=1)
                     ->  Nested Loop Left Join  (cost=0.00..26695.12 rows=67145 width=442) (actual time=0.053..0.468 rows=138 loops=1)
                           ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.00..6118.15 rows=67145 width=438) (actual time=0.007..0.06
9 rows=138 loops=1)
                           ->  Index Only Scan using accounts_pkey on accounts  (cost=0.00..0.30 rows=1 width=4) (actual time=0.002..0.002 rows=1 loop
s=138)
                                 Index Cond: (id = statuses.account_id)
                                 Heap Fetches: 140
                     ->  Index Only Scan using index_follows_on_account_id_and_target_account_id on follows  (cost=0.00..1.60 rows=2 width=8) (actual 
time=0.131..0.131 rows=1 loops=138)
                           Index Cond: (target_account_id = accounts.id)
                           Heap Fetches: 101
               ->  Index Scan using index_mentions_on_status_id on mentions  (cost=0.00..0.28 rows=1 width=12) (actual time=0.001..0.001 rows=0 loops=
139)
                     Index Cond: (status_id = statuses.id)
 Total runtime: 18.965 ms
(17 rows)
Collaborator

akihikodaki commented Jun 18, 2017

@alpaca-tc Could you test the following query? I'm not really confident with the performance of the query. This query change is related to #2967.

> EXPLAIN ANALYZE SELECT "statuses".* FROM "statuses" LEFT OUTER JOIN "accounts" ON "accounts"."id" = "statuses"."account_id" LEFT OUTER JOIN "follows" ON "follows"."target_account_id" = "accounts"."id" LEFT OUTER JOIN "accounts" "followers_accounts" ON "followers_accounts"."id" = "follows"."account_id" LEFT OUTER JOIN "mentions" ON "mentions"."status_id" = "statuses"."id" WHERE ((("statuses"."visibility" != 3) OR "mentions"."account_id" = 1) AND "follows"."account_id" = 1 OR "statuses"."account_id" = 1) GROUP BY "statuses"."id" ORDER BY "statuses"."id" DESC LIMIT 100;
                                                                                        QUERY PLAN                                                    
                                    
------------------------------------------------------------------------------------------------------------------------------------------------------
------------------------------------
 Limit  (cost=0.00..380.01 rows=100 width=438) (actual time=17.460..18.884 rows=100 loops=1)
   ->  Group  (cost=0.00..155684.60 rows=40969 width=438) (actual time=17.459..18.870 rows=100 loops=1)
         ->  Nested Loop Left Join  (cost=0.00..155582.18 rows=40969 width=438) (actual time=17.456..18.767 rows=105 loops=1)
               Filter: ((((statuses.visibility <> 3) OR (mentions.account_id = 1)) AND (follows.account_id = 1)) OR (statuses.account_id = 1))
               Rows Removed by Filter: 45
               ->  Nested Loop Left Join  (cost=0.00..135363.65 rows=67145 width=442) (actual time=17.445..18.552 rows=139 loops=1)
                     ->  Nested Loop Left Join  (cost=0.00..26695.12 rows=67145 width=442) (actual time=0.053..0.468 rows=138 loops=1)
                           ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.00..6118.15 rows=67145 width=438) (actual time=0.007..0.06
9 rows=138 loops=1)
                           ->  Index Only Scan using accounts_pkey on accounts  (cost=0.00..0.30 rows=1 width=4) (actual time=0.002..0.002 rows=1 loop
s=138)
                                 Index Cond: (id = statuses.account_id)
                                 Heap Fetches: 140
                     ->  Index Only Scan using index_follows_on_account_id_and_target_account_id on follows  (cost=0.00..1.60 rows=2 width=8) (actual 
time=0.131..0.131 rows=1 loops=138)
                           Index Cond: (target_account_id = accounts.id)
                           Heap Fetches: 101
               ->  Index Scan using index_mentions_on_status_id on mentions  (cost=0.00..0.28 rows=1 width=12) (actual time=0.001..0.001 rows=0 loops=
139)
                     Index Cond: (status_id = statuses.id)
 Total runtime: 18.965 ms
(17 rows)
@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 19, 2017

Collaborator

c5b4016 updated the query and edited #3842 (comment) as well.

Collaborator

akihikodaki commented Jun 19, 2017

c5b4016 updated the query and edited #3842 (comment) as well.

Filter direct statuses in Status.as_home_timeline
The classes using Status.as_home_timeline, namely Feed and
PrecomputeFeedService are expected to filter direct statuses as
FanOutWriteService does, but their filtering were incomplete or missing.

This commit solves the problem by filtering direct statuses in
as_home_timeline as the other similar methods such as as_public_timeline
does.

@Gargron Gargron merged commit bab5a18 into tootsuite:master Jun 20, 2017

2 checks passed

codeclimate no new or fixed issues
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Gargron added a commit that referenced this pull request Jun 20, 2017

Gargron added a commit that referenced this pull request Jun 20, 2017

Revert "Don't attach IntersectionObserver for wrapped statuses" (#3877)
* Revert "Bump version to 1.4.4"

This reverts commit 1585b0c.

* Revert "Fix conversations (fixes #3869) (#3870)"

This reverts commit 15b43f5.

* Revert "Fix streaming server. Redis connection subscribe for each channel. (#3828)"

This reverts commit d8ec832.

* Revert "Filter direct statuses in Status.as_home_timeline (#3842)"

This reverts commit bab5a18.

* Revert "Fix RemoteFollow behavior (#3868)"

This reverts commit a20cf3b.

* Revert "Update fabricator for MediaAttachment to attach a file according to type (#3862)"

This reverts commit 356df7a.

* Revert "Upgrade React Router (#3677)"

This reverts commit 8f03fdc.

* Revert "Do not call setState from unmounted component (#3853)"

This reverts commit 1fc6cb4.

* Revert "Replace TextIconButton for SensitiveButton to IconButton (#3759)"

This reverts commit eb832e8.

* Revert "Fix RTL detection on Ruby side (#3867)"

This reverts commit b16b693.

* Revert "i18n: Fixed typo in Polish translation (#3864)"

This reverts commit da6fa02.

* Revert "Don't attach IntersectionObserver for wrapped statuses (#3863)"

This reverts commit 94ad070.
@alpaca-tc

This comment has been minimized.

Show comment
Hide comment
@alpaca-tc

alpaca-tc Jun 21, 2017

Contributor

@akihikodaki My apologies for the late reply. here you are.

https://pawoo.net/web/accounts/1

Limit  (cost=1.72..874401.38 rows=100 width=332) (actual time=70.674..9965.812 rows=100 loops=1)
  ->  Group  (cost=1.72..113645725.49 rows=12997 width=332) (actual time=70.672..9965.745 rows=100 loops=1)
        Group Key: statuses.id
        ->  Nested Loop Left Join  (cost=1.72..113645693.00 rows=12997 width=332) (actual time=70.670..9965.443 rows=105 loops=1)
              Filter: ((((statuses.visibility <> 3) OR (mentions.account_id = 1)) AND (follows.account_id = 1)) OR (statuses.account_id = 1))
              Rows Removed by Filter: 6189510
              ->  Nested Loop Left Join  (cost=0.87..12380809.91 rows=21446578 width=336) (actual time=0.015..301.481 rows=78800 loops=1)
                    ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.44..1522700.58 rows=21446578 width=332) (actual time=0.009..66.121 rows=76118 loops=1)
                    ->  Index Scan using index_mentions_on_status_id on mentions  (cost=0.43..0.49 rows=2 width=12) (actual time=0.002..0.002 rows=0 loops=76118)
                          Index Cond: (status_id = statuses.id)
              ->  Nested Loop Left Join  (cost=0.85..4.60 rows=6 width=8) (actual time=0.008..0.099 rows=79 loops=78800)
                    ->  Index Only Scan using accounts_pkey on accounts  (cost=0.42..0.44 rows=1 width=4) (actual time=0.003..0.003 rows=1 loops=78800)
                          Index Cond: (id = statuses.account_id)
                          Heap Fetches: 57714
                    ->  Index Scan using index_follows_on_target_account_id on follows  (cost=0.43..3.03 rows=113 width=8) (actual time=0.004..0.064 rows=78 loops=78800)
                          Index Cond: (target_account_id = accounts.id)
Planning time: 0.644 ms
Execution time: 9965.895 ms

https://pawoo.net/web/accounts/34

Limit  (cost=1.72..874377.10 rows=100 width=332) (actual time=7.373..458.562 rows=100 loops=1)
  ->  Group  (cost=1.72..113651313.56 rows=12998 width=332) (actual time=7.373..458.507 rows=100 loops=1)
        Group Key: statuses.id
        ->  Nested Loop Left Join  (cost=1.72..113651281.06 rows=12998 width=332) (actual time=7.370..458.260 rows=100 loops=1)
              Filter: ((((statuses.visibility <> 3) OR (mentions.account_id = 34)) AND (follows.account_id = 34)) OR (statuses.account_id = 34))
              Rows Removed by Filter: 243110
              ->  Nested Loop Left Join  (cost=0.87..12381398.85 rows=21447637 width=336) (actual time=0.015..19.479 rows=4738 loops=1)
                    ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.44..1522772.91 rows=21447637 width=332) (actual time=0.009..4.247 rows=4592 loops=1)
                    ->  Index Scan using index_mentions_on_status_id on mentions  (cost=0.43..0.49 rows=2 width=12) (actual time=0.002..0.002 rows=0 loops=4592)
                          Index Cond: (status_id = statuses.id)
              ->  Nested Loop Left Join  (cost=0.85..4.60 rows=6 width=8) (actual time=0.009..0.076 rows=51 loops=4738)
                    ->  Index Only Scan using accounts_pkey on accounts  (cost=0.42..0.44 rows=1 width=4) (actual time=0.003..0.004 rows=1 loops=4738)
                          Index Cond: (id = statuses.account_id)
                          Heap Fetches: 4751
                    ->  Index Scan using index_follows_on_target_account_id on follows  (cost=0.43..3.03 rows=113 width=8) (actual time=0.004..0.049 rows=51 loops=4738)
                          Index Cond: (target_account_id = accounts.id)
Planning time: 0.660 ms
Execution time: 458.655 ms
Contributor

alpaca-tc commented Jun 21, 2017

@akihikodaki My apologies for the late reply. here you are.

https://pawoo.net/web/accounts/1

Limit  (cost=1.72..874401.38 rows=100 width=332) (actual time=70.674..9965.812 rows=100 loops=1)
  ->  Group  (cost=1.72..113645725.49 rows=12997 width=332) (actual time=70.672..9965.745 rows=100 loops=1)
        Group Key: statuses.id
        ->  Nested Loop Left Join  (cost=1.72..113645693.00 rows=12997 width=332) (actual time=70.670..9965.443 rows=105 loops=1)
              Filter: ((((statuses.visibility <> 3) OR (mentions.account_id = 1)) AND (follows.account_id = 1)) OR (statuses.account_id = 1))
              Rows Removed by Filter: 6189510
              ->  Nested Loop Left Join  (cost=0.87..12380809.91 rows=21446578 width=336) (actual time=0.015..301.481 rows=78800 loops=1)
                    ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.44..1522700.58 rows=21446578 width=332) (actual time=0.009..66.121 rows=76118 loops=1)
                    ->  Index Scan using index_mentions_on_status_id on mentions  (cost=0.43..0.49 rows=2 width=12) (actual time=0.002..0.002 rows=0 loops=76118)
                          Index Cond: (status_id = statuses.id)
              ->  Nested Loop Left Join  (cost=0.85..4.60 rows=6 width=8) (actual time=0.008..0.099 rows=79 loops=78800)
                    ->  Index Only Scan using accounts_pkey on accounts  (cost=0.42..0.44 rows=1 width=4) (actual time=0.003..0.003 rows=1 loops=78800)
                          Index Cond: (id = statuses.account_id)
                          Heap Fetches: 57714
                    ->  Index Scan using index_follows_on_target_account_id on follows  (cost=0.43..3.03 rows=113 width=8) (actual time=0.004..0.064 rows=78 loops=78800)
                          Index Cond: (target_account_id = accounts.id)
Planning time: 0.644 ms
Execution time: 9965.895 ms

https://pawoo.net/web/accounts/34

Limit  (cost=1.72..874377.10 rows=100 width=332) (actual time=7.373..458.562 rows=100 loops=1)
  ->  Group  (cost=1.72..113651313.56 rows=12998 width=332) (actual time=7.373..458.507 rows=100 loops=1)
        Group Key: statuses.id
        ->  Nested Loop Left Join  (cost=1.72..113651281.06 rows=12998 width=332) (actual time=7.370..458.260 rows=100 loops=1)
              Filter: ((((statuses.visibility <> 3) OR (mentions.account_id = 34)) AND (follows.account_id = 34)) OR (statuses.account_id = 34))
              Rows Removed by Filter: 243110
              ->  Nested Loop Left Join  (cost=0.87..12381398.85 rows=21447637 width=336) (actual time=0.015..19.479 rows=4738 loops=1)
                    ->  Index Scan Backward using statuses_pkey on statuses  (cost=0.44..1522772.91 rows=21447637 width=332) (actual time=0.009..4.247 rows=4592 loops=1)
                    ->  Index Scan using index_mentions_on_status_id on mentions  (cost=0.43..0.49 rows=2 width=12) (actual time=0.002..0.002 rows=0 loops=4592)
                          Index Cond: (status_id = statuses.id)
              ->  Nested Loop Left Join  (cost=0.85..4.60 rows=6 width=8) (actual time=0.009..0.076 rows=51 loops=4738)
                    ->  Index Only Scan using accounts_pkey on accounts  (cost=0.42..0.44 rows=1 width=4) (actual time=0.003..0.004 rows=1 loops=4738)
                          Index Cond: (id = statuses.account_id)
                          Heap Fetches: 4751
                    ->  Index Scan using index_follows_on_target_account_id on follows  (cost=0.43..3.03 rows=113 width=8) (actual time=0.004..0.049 rows=51 loops=4738)
                          Index Cond: (target_account_id = accounts.id)
Planning time: 0.660 ms
Execution time: 458.655 ms
@nullkal

This comment has been minimized.

Show comment
Hide comment
@nullkal

nullkal Jun 21, 2017

Collaborator

I updated mstdn.jp to mastodon 1.4.4 and I saw long running queries I suspect this change is concerned.

image

Collaborator

nullkal commented Jun 21, 2017

I updated mstdn.jp to mastodon 1.4.4 and I saw long running queries I suspect this change is concerned.

image

@alpaca-tc

This comment has been minimized.

Show comment
Hide comment
@alpaca-tc

alpaca-tc Jun 21, 2017

Contributor

I just try to optimize the query. Please give me some advice if you have ideas or suggestions :)

Contributor

alpaca-tc commented Jun 21, 2017

I just try to optimize the query. Please give me some advice if you have ideas or suggestions :)

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 21, 2017

Collaborator

The sequence of joining with follows, filtering with follows, joining with mentions if required, and then filtering mentions could be faster.
To achieve that, we need to tell the planner to join follows eariler than mentions, but I don't know how to do that.

Collaborator

akihikodaki commented Jun 21, 2017

The sequence of joining with follows, filtering with follows, joining with mentions if required, and then filtering mentions could be faster.
To achieve that, we need to tell the planner to join follows eariler than mentions, but I don't know how to do that.

@alpaca-tc

This comment has been minimized.

Show comment
Hide comment
@alpaca-tc

alpaca-tc Jun 21, 2017

Contributor

@akihikodaki How about the following query? This is still work in progress.

WITH following_account_ids AS (
  SELECT target_account_id
  FROM follows
  WHERE account_id = 34
), 
mentioned_status_ids AS (
  SELECT "mentions"."status_id" FROM mentions WHERE "mentions"."account_id" = 34
)

SELECT "statuses".*
FROM "statuses"
WHERE "statuses"."account_id" = 34 /* own statuses */
   OR ("statuses"."visibility" IN (0, 1, 2) AND "statuses"."account_id" IN (SELECT * FROM following_account_ids)) /* followings */
   OR ("statuses"."visibility" = 3          AND "statuses"."id" IN (SELECT * FROM mentioned_status_ids)) /* DM statuses */
ORDER BY "statuses"."id" DESC
LIMIT 200

account = Account.find(XXX)

following_account_ids = Follow.select(:target_account_id).where(account: account)
mentioned_status_ids = Mention.select(:status_id).where(account: account)

own_statuses = Status.where(account: account)
following_statuses = Status.where(visibility: [:public, :unlisted, :private]).where(account_id: following_account_ids)
direct_statuses = Status.where(visibility: [:direct]).where(id: mentioned_status_ids)

own_statuses.or(following_statuses).or(direct_statuses).limit(200)

(Oh, I should filter direct statuses by followings. 😕 )

Contributor

alpaca-tc commented Jun 21, 2017

@akihikodaki How about the following query? This is still work in progress.

WITH following_account_ids AS (
  SELECT target_account_id
  FROM follows
  WHERE account_id = 34
), 
mentioned_status_ids AS (
  SELECT "mentions"."status_id" FROM mentions WHERE "mentions"."account_id" = 34
)

SELECT "statuses".*
FROM "statuses"
WHERE "statuses"."account_id" = 34 /* own statuses */
   OR ("statuses"."visibility" IN (0, 1, 2) AND "statuses"."account_id" IN (SELECT * FROM following_account_ids)) /* followings */
   OR ("statuses"."visibility" = 3          AND "statuses"."id" IN (SELECT * FROM mentioned_status_ids)) /* DM statuses */
ORDER BY "statuses"."id" DESC
LIMIT 200

account = Account.find(XXX)

following_account_ids = Follow.select(:target_account_id).where(account: account)
mentioned_status_ids = Mention.select(:status_id).where(account: account)

own_statuses = Status.where(account: account)
following_statuses = Status.where(visibility: [:public, :unlisted, :private]).where(account_id: following_account_ids)
direct_statuses = Status.where(visibility: [:direct]).where(id: mentioned_status_ids)

own_statuses.or(following_statuses).or(direct_statuses).limit(200)

(Oh, I should filter direct statuses by followings. 😕 )

@alpaca-tc

This comment has been minimized.

Show comment
Hide comment
@alpaca-tc

alpaca-tc Jun 21, 2017

Contributor

@akihikodaki IMO, to separate query is the best way. Could you review the following?

def as_home_timeline(account, limit)
  following_ids = Follow.where(account: account).pluck(:target_account_id)
  following_statuses_scope = Status.where(account: following_ids)

  account_statuses = Status.where(account: account)
  following_statuses = following_statuses_scope.where(visibility: [:public, :unlisted, :private])
  mentioned_statuses = following_statuses_scope.where(visibility: :direct).joins(:mentions).where(mentions: { account: account })

  ids = account_statuses.limit(limit).ids + following_statuses.limit(limit).ids + mentioned_statuses.limit(limit).pluck(:status_id)
  Status.where(id: ids).limit(limit)
end
Contributor

alpaca-tc commented Jun 21, 2017

@akihikodaki IMO, to separate query is the best way. Could you review the following?

def as_home_timeline(account, limit)
  following_ids = Follow.where(account: account).pluck(:target_account_id)
  following_statuses_scope = Status.where(account: following_ids)

  account_statuses = Status.where(account: account)
  following_statuses = following_statuses_scope.where(visibility: [:public, :unlisted, :private])
  mentioned_statuses = following_statuses_scope.where(visibility: :direct).joins(:mentions).where(mentions: { account: account })

  ids = account_statuses.limit(limit).ids + following_statuses.limit(limit).ids + mentioned_statuses.limit(limit).pluck(:status_id)
  Status.where(id: ids).limit(limit)
end
@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 21, 2017

Collaborator

If we could use a raw query, join_collapse_limit would be the simplest and best solution. However, personally I'm not favor of a raw query and wonder if it can still be represented in one query. Subquery could be a solution.

Collaborator

akihikodaki commented Jun 21, 2017

If we could use a raw query, join_collapse_limit would be the simplest and best solution. However, personally I'm not favor of a raw query and wonder if it can still be represented in one query. Subquery could be a solution.

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 21, 2017

Collaborator

By the way, you should be careful for

  • the order and
  • the limit

in case you merge multiple records. The order could be resolved with simple merge sort. The limit could be enhanced with where('statuses.id > ?', the_records_we_already_have.last.id) if the_records_we_already_have.size >= limit or something kind of.

Collaborator

akihikodaki commented Jun 21, 2017

By the way, you should be careful for

  • the order and
  • the limit

in case you merge multiple records. The order could be resolved with simple merge sort. The limit could be enhanced with where('statuses.id > ?', the_records_we_already_have.last.id) if the_records_we_already_have.size >= limit or something kind of.

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 21, 2017

Collaborator

Here is my latest query.

where.not(visibility: :direct, breaking: true)
  .or(where(id: Mention.select(:status_id).where(account: account).limit(100)))
  .where(follows: { account_id: account })
  .or(references(:follows).where(account: account))
  .left_outer_joins(account: :followers).group(:id).limit(100)
Collaborator

akihikodaki commented Jun 21, 2017

Here is my latest query.

where.not(visibility: :direct, breaking: true)
  .or(where(id: Mention.select(:status_id).where(account: account).limit(100)))
  .where(follows: { account_id: account })
  .or(references(:follows).where(account: account))
  .left_outer_joins(account: :followers).group(:id).limit(100)
@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Jun 21, 2017

Member

I would be careful with limits, since PrecomputeFeedService wants to iterate over many more items (e.g. restore ~200 items in the home feed) than the live query. Perhaps for that purpose the methods should be split, or use an argument to decide whether to apply limits.

Member

Gargron commented Jun 21, 2017

I would be careful with limits, since PrecomputeFeedService wants to iterate over many more items (e.g. restore ~200 items in the home feed) than the live query. Perhaps for that purpose the methods should be split, or use an argument to decide whether to apply limits.

@akihikodaki

This comment has been minimized.

Show comment
Hide comment
@akihikodaki

akihikodaki Jun 21, 2017

Collaborator

I'm adding such an argument to as-timeline methods since it is usable for this case and for Redis. Currently we have Feed model, but I think it should be as_home_timeline and from_database and from_redis should respectively be as_home_timeline_from_database and as_home_timeline_from_redis. All of them would have the argument for consistency.

Collaborator

akihikodaki commented Jun 21, 2017

I'm adding such an argument to as-timeline methods since it is usable for this case and for Redis. Currently we have Feed model, but I think it should be as_home_timeline and from_database and from_redis should respectively be as_home_timeline_from_database and as_home_timeline_from_redis. All of them would have the argument for consistency.

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Jun 21, 2017

Member

mastodon.social is also currently having multiple long running queries (>650,000ms!) so I will probably have to tag a 1.4.5 when this is fixed before announcing the new release (rather than announcing 1.4.4, which I still haven't done publicly from my account/Mastodon account)

Member

Gargron commented Jun 21, 2017

mastodon.social is also currently having multiple long running queries (>650,000ms!) so I will probably have to tag a 1.4.5 when this is fixed before announcing the new release (rather than announcing 1.4.4, which I still haven't done publicly from my account/Mastodon account)

Gargron added a commit that referenced this pull request Jun 21, 2017

Fix regression from #3842
Simplify the query by omitting all direct statuses. Private statuses
are allowed because they are from accounts we are following (so
by definition)

Resolves #3887 (alternative)
@nightpool

This comment has been minimized.

Show comment
Hide comment
@nightpool

nightpool Jun 21, 2017

Collaborator

@Gargron is there no timeout on those queries/requests/workers? that seems like a more important thing to get fixed O.O

Collaborator

nightpool commented Jun 21, 2017

@Gargron is there no timeout on those queries/requests/workers? that seems like a more important thing to get fixed O.O

@Gargron

This comment has been minimized.

Show comment
Hide comment
@Gargron

Gargron Jun 21, 2017

Member

@nightpool There is a 90 sec timeout on request workers, however timeouts are a tricky business, because by interrupting the execution of code in arbitrary places you might end up with weird states (although transactions should take care of the worst cases). Also, if somebody is uploading a file on a slow connection, the timeout may give insufficient time, this is why currently it's so big (90 sec)

Background workers have no timeouts by definition.

There is an option of "statement timeouts" in Rails for the database connection, however the setting is session-wide, which doesn't work with pgBouncer in transaction mode. The statement timeout can be set on pgBouncer level or Postgres level manually, however I do not know if that's a good idea, what value to take, and in any case it doesn't help most admins since it's outside of Mastodon itself.

Member

Gargron commented Jun 21, 2017

@nightpool There is a 90 sec timeout on request workers, however timeouts are a tricky business, because by interrupting the execution of code in arbitrary places you might end up with weird states (although transactions should take care of the worst cases). Also, if somebody is uploading a file on a slow connection, the timeout may give insufficient time, this is why currently it's so big (90 sec)

Background workers have no timeouts by definition.

There is an option of "statement timeouts" in Rails for the database connection, however the setting is session-wide, which doesn't work with pgBouncer in transaction mode. The statement timeout can be set on pgBouncer level or Postgres level manually, however I do not know if that's a good idea, what value to take, and in any case it doesn't help most admins since it's outside of Mastodon itself.

Gargron added a commit that referenced this pull request Jun 22, 2017

Fix regression from #3842 (#3892)
* Fix regression from #3842

Simplify the query by omitting all direct statuses. Private statuses
are allowed because they are from accounts we are following (so
by definition)

Resolves #3887 (alternative)

* Adjust test

koteitan added a commit to koteitan/mastodon that referenced this pull request Jun 25, 2017

Filter direct statuses in Status.as_home_timeline (tootsuite#3842)
The classes using Status.as_home_timeline, namely Feed and
PrecomputeFeedService are expected to filter direct statuses as
FanOutWriteService does, but their filtering were incomplete or missing.

This commit solves the problem by filtering direct statuses in
as_home_timeline as the other similar methods such as as_public_timeline
does.

koteitan added a commit to koteitan/mastodon that referenced this pull request Jun 25, 2017

Revert "Don't attach IntersectionObserver for wrapped statuses" (toot…
…suite#3877)

* Revert "Bump version to 1.4.4"

This reverts commit 1585b0c.

* Revert "Fix conversations (fixes tootsuite#3869) (tootsuite#3870)"

This reverts commit 15b43f5.

* Revert "Fix streaming server. Redis connection subscribe for each channel. (tootsuite#3828)"

This reverts commit d8ec832.

* Revert "Filter direct statuses in Status.as_home_timeline (tootsuite#3842)"

This reverts commit bab5a18.

* Revert "Fix RemoteFollow behavior (tootsuite#3868)"

This reverts commit a20cf3b.

* Revert "Update fabricator for MediaAttachment to attach a file according to type (tootsuite#3862)"

This reverts commit 356df7a.

* Revert "Upgrade React Router (tootsuite#3677)"

This reverts commit 8f03fdc.

* Revert "Do not call setState from unmounted component (tootsuite#3853)"

This reverts commit 1fc6cb4.

* Revert "Replace TextIconButton for SensitiveButton to IconButton (tootsuite#3759)"

This reverts commit eb832e8.

* Revert "Fix RTL detection on Ruby side (tootsuite#3867)"

This reverts commit b16b693.

* Revert "i18n: Fixed typo in Polish translation (tootsuite#3864)"

This reverts commit da6fa02.

* Revert "Don't attach IntersectionObserver for wrapped statuses (tootsuite#3863)"

This reverts commit 94ad070.

koteitan added a commit to koteitan/mastodon that referenced this pull request Jun 25, 2017

Fix regression from tootsuite#3842 (tootsuite#3892)
* Fix regression from tootsuite#3842

Simplify the query by omitting all direct statuses. Private statuses
are allowed because they are from accounts we are following (so
by definition)

Resolves tootsuite#3887 (alternative)

* Adjust test

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

Filter direct statuses in Status.as_home_timeline (tootsuite#3842)
The classes using Status.as_home_timeline, namely Feed and
PrecomputeFeedService are expected to filter direct statuses as
FanOutWriteService does, but their filtering were incomplete or missing.

This commit solves the problem by filtering direct statuses in
as_home_timeline as the other similar methods such as as_public_timeline
does.

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

Revert "Don't attach IntersectionObserver for wrapped statuses" (toot…
…suite#3877)

* Revert "Bump version to 1.4.4"

This reverts commit 1585b0c.

* Revert "Fix conversations (fixes tootsuite#3869) (tootsuite#3870)"

This reverts commit 15b43f5.

* Revert "Fix streaming server. Redis connection subscribe for each channel. (tootsuite#3828)"

This reverts commit d8ec832.

* Revert "Filter direct statuses in Status.as_home_timeline (tootsuite#3842)"

This reverts commit bab5a18.

* Revert "Fix RemoteFollow behavior (tootsuite#3868)"

This reverts commit a20cf3b.

* Revert "Update fabricator for MediaAttachment to attach a file according to type (tootsuite#3862)"

This reverts commit 356df7a.

* Revert "Upgrade React Router (tootsuite#3677)"

This reverts commit 8f03fdc.

* Revert "Do not call setState from unmounted component (tootsuite#3853)"

This reverts commit 1fc6cb4.

* Revert "Replace TextIconButton for SensitiveButton to IconButton (tootsuite#3759)"

This reverts commit eb832e8.

* Revert "Fix RTL detection on Ruby side (tootsuite#3867)"

This reverts commit b16b693.

* Revert "i18n: Fixed typo in Polish translation (tootsuite#3864)"

This reverts commit da6fa02.

* Revert "Don't attach IntersectionObserver for wrapped statuses (tootsuite#3863)"

This reverts commit 94ad070.

YaQ00 added a commit to YaQ00/mastodon that referenced this pull request Sep 5, 2017

Fix regression from tootsuite#3842 (tootsuite#3892)
* Fix regression from tootsuite#3842

Simplify the query by omitting all direct statuses. Private statuses
are allowed because they are from accounts we are following (so
by definition)

Resolves tootsuite#3887 (alternative)

* Adjust test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment