From f49b3c439faae960a064c4f076794ea81429c827 Mon Sep 17 00:00:00 2001 From: evazion Date: Tue, 27 Sep 2022 06:12:34 -0500 Subject: [PATCH] posts: optimize modqueue page, status:modqueue, and status:unmoderated searches. * Optimize status:modqueue and status:unmoderated searches. This brings them down from taking 500ms-1000ms per search to ~5ms. * Change status:unmoderated so that it only filters out the user's disapproved posts, not the user's own uploads or past approvals. Now it's equivalent to `status:modqueue -disapproved:evazion`, whereas before it was equivalent to `status:modqueue -disapproved:evazion -approver:evazion -user:evazion`. Filtering out the user's own uploads and approvals was slow and usually unnecessary, since for most users it's rare for their own uploads or approvals to reenter the modqueue. Before status:modqueue did this: SELECT * FROM posts WHERE is_pending = TRUE OR is_flagged = TRUE OR (is_deleted = TRUE AND id IN (SELECT post_id FROM post_appeals WHERE status = 0)) Now we do this: SELECT * FROM posts WHERE id IN (SELECT id FROM posts WHERE is_pending = TRUE UNION ALL SELECT id FROM posts WHERE is_flagged = TRUE UNION ALL SELECT id FROM posts WHERE id IN (SELECT post_id FROM post_appeals WHERE status = 0)) Postgres had a bad time with the "pending or flagged or has a pending appeal" clause because it didn't know that posts can only be in one state at a time, so it overestimated how many posts would be returned and chose a seq scan. Replacing the OR with a UNION avoids this. --- app/controllers/modqueue_controller.rb | 2 +- app/logical/concerns/searchable.rb | 11 +++++++++++ app/models/post.rb | 13 +++++-------- test/unit/post_disapproval_test.rb | 23 ++++++++--------------- 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/app/controllers/modqueue_controller.rb b/app/controllers/modqueue_controller.rb index d10728483..1255a1dac 100644 --- a/app/controllers/modqueue_controller.rb +++ b/app/controllers/modqueue_controller.rb @@ -6,7 +6,7 @@ class ModqueueController < ApplicationController def index authorize :modqueue - @posts = Post.includes(:appeals, :disapprovals, :uploader, :media_asset, flags: [:creator]).in_modqueue.available_for_moderation(CurrentUser.user, hidden: search_params[:hidden]) + @posts = Post.includes(:appeals, :disapprovals, :uploader, :media_asset, flags: [:creator]).available_for_moderation(CurrentUser.user, hidden: search_params[:hidden]) @modqueue_posts = @posts.reselect(nil).reorder(nil).offset(nil).limit(nil) @posts = @posts.paginated_search(params, count_pages: true, count: @modqueue_posts.to_a.size, defaults: { order: "modqueue" }) diff --git a/app/logical/concerns/searchable.rb b/app/logical/concerns/searchable.rb index f5c3110aa..6f939c387 100644 --- a/app/logical/concerns/searchable.rb +++ b/app/logical/concerns/searchable.rb @@ -135,6 +135,17 @@ module Searchable where_numeric_matches(qualified_column, value) end + # where_union(A, B, C) is like `WHERE A OR B OR C`, except it may be faster if the conditions are disjoint. + # where_union(A, B) does `SELECT * FROM table WHERE id IN (SELECT id FROM table WHERE A UNION ALL SELECT id FROM table WHERE B)` + def where_union(*relations, primary_key: :id, foreign_key: :id) + arels = relations.map { |relation| relation.select(foreign_key).arel } + union = arels.reduce do |left, right| + Arel::Nodes::UnionAll.new(left, right) + end + + where(arel_table[primary_key].in(union)) + end + # @param attr [String] the name of the JSON field # @param hash [Hash] the hash of values it should contain def where_json_contains(attr, hash) diff --git a/app/models/post.rb b/app/models/post.rb index 7d40a8f6e..25488c84c 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -80,8 +80,8 @@ class Post < ApplicationRecord scope :banned, -> { where(is_banned: true) } # XXX conflict with deletable scope :active, -> { where(is_pending: false, is_deleted: false, is_flagged: false).where.not(id: PostAppeal.pending) } - scope :appealed, -> { deleted.where(id: PostAppeal.pending.select(:post_id)) } - scope :in_modqueue, -> { pending.or(flagged).or(appealed) } + scope :appealed, -> { where(id: PostAppeal.pending.select(:post_id)) } + scope :in_modqueue, -> { where_union(pending, flagged, appealed) } scope :expired, -> { pending.where("posts.created_at < ?", Danbooru.config.moderation_period.ago) } scope :unflagged, -> { where(is_flagged: false) } @@ -1049,15 +1049,12 @@ class Post < ApplicationRecord end def available_for_moderation(user, hidden: false) - return none if user.is_anonymous? - - approved_posts = user.post_approvals.select(:post_id) disapproved_posts = user.post_disapprovals.select(:post_id) if hidden.present? - where("posts.uploader_id = ? OR posts.id IN (#{approved_posts.to_sql}) OR posts.id IN (#{disapproved_posts.to_sql})", user.id) + in_modqueue.where(id: disapproved_posts) else - where.not(uploader: user).where.not(id: approved_posts).where.not(id: disapproved_posts) + in_modqueue.where.not(id: disapproved_posts) end end @@ -1124,7 +1121,7 @@ class Post < ApplicationRecord when "active" active when "unmoderated" - in_modqueue.available_for_moderation(current_user, hidden: false) + available_for_moderation(current_user, hidden: false) when "all", "any" where("TRUE") else diff --git a/test/unit/post_disapproval_test.rb b/test/unit/post_disapproval_test.rb index 9ba87fbae..402945b73 100644 --- a/test/unit/post_disapproval_test.rb +++ b/test/unit/post_disapproval_test.rb @@ -3,7 +3,7 @@ require "test_helper" class PostDisapprovalTest < ActiveSupport::TestCase context "In all cases" do setup do - @alice = FactoryBot.create(:moderator_user, name: "alice") + @alice = FactoryBot.create(:moderator_user) CurrentUser.user = @alice end @@ -28,25 +28,18 @@ class PostDisapprovalTest < ActiveSupport::TestCase end context "when the current user is alice" do - setup do - CurrentUser.user = @alice - end - should "remove the associated post from alice's moderation queue" do - assert_not(Post.available_for_moderation(CurrentUser.user, hidden: false).map(&:id).include?(@post_1.id)) - assert(Post.available_for_moderation(CurrentUser.user, hidden: false).map(&:id).include?(@post_2.id)) + assert_not(Post.available_for_moderation(@alice, hidden: false).map(&:id).include?(@post_1.id)) + assert(Post.available_for_moderation(@alice, hidden: false).map(&:id).include?(@post_2.id)) end end - context "when the current user is brittony" do - setup do - @brittony = FactoryBot.create(:moderator_user) - CurrentUser.user = @brittony - end + context "when the current user is not the disapprover" do + should "not remove the associated post from the disapprover's moderation queue" do + @mod = create(:moderator_user) - should "not remove the associated post from brittony's moderation queue" do - assert(Post.available_for_moderation(CurrentUser.user, hidden: false).map(&:id).include?(@post_1.id)) - assert(Post.available_for_moderation(CurrentUser.user, hidden: false).map(&:id).include?(@post_2.id)) + assert(Post.available_for_moderation(@mod, hidden: false).map(&:id).include?(@post_1.id)) + assert(Post.available_for_moderation(@mod, hidden: false).map(&:id).include?(@post_2.id)) end end end