PERF: Prefer joins for `User#private_posts_for_user` take 2. (PR #10452)

The subquery here prevents the planner from optimizing the query. As such, we prefer joining against the requried tables instead.

GitHub

@davidtaylorhq Could you review this since for me? Thanks!

When two posts in a topic has the same rank, we should order by the latest post.

It seems a shame to lose the centralised PRIVATE_MESSAGES_SQL… is there some way we can optimize that SQL rather than re-implementing it in two places :thinking:

Is post_search_data needed here? This seems like a search-specific thing so maybe it shouldn’t be in the scope?

Yea it is required because I’m joining now which does work with active record or.

The SQL itself isn’t the problem though. It is the use of the subquery that throws the planner off.

Where are we using activerecord or? I think this commit removes it from search.rb?

My main worry is that we now have a scope called Post.all_posts_for_user which could be a little misleading because:

  • It joins to post_search_data. Which makes sense for the search use case… but otherwise is a bit weird
  • It checks access permissions for PMs, but does not check category/whisper permissions for regular topics. I worry someone might use this, thinking it’s secure, and then we have a data leak

A more specific name might help

scope :insecure_posts_and_secure_private_messages_for_user_with_search_data

But clearly that’s getting a bit ridiculous :stuck_out_tongue_winking_eye:

Since this is search-specific, could we keep it in search.rb instead?

1 Like

@davidtaylorhq I’m going to close this first because I realized that my query could return duplicated topic_ids.

@davidtaylorhq Thank you for raising some very valid points. I’m working on breaking down my PR https://github.com/discourse/discourse/pull/10436 into smaller pieces.