From 97143efc521df616c202f7a5bb98c85ba3bcc5e2 Mon Sep 17 00:00:00 2001 From: Alan Guo Xiang Tan Date: Fri, 13 Sep 2024 09:04:02 +0800 Subject: [PATCH] PERF: Drop `user_search_similar_results` site setting (#28874) In 14cf8eacf1a679c08ea7df93aff17949d1a9c4df, we added the `user_search_similar_results` site setting which when enabled will use trigram matching for similarity search in `UserSearch`. However, we noted that adding the `index_users_on_username_lower_trgm` index is causing the PG planner to not use the `index_users_on_username_lower` index when the `=` operator is used against the `username_lower` column. Based on the PG mailing list discussion where support for the `=` operator in gist_trgm_ops was being considered, it stated that "I also have checked that btree_gist is preferred over pg_trgm gist index for equality search." This is however quite different from reality on our own PG clusters where the btree index is not preferred leading to significantly slower queries when the `=` operator is used. Since the pg_trgm gist index is only used for queries when the `user_search_similar_results` site setting is enabled, we decided to drop the feature instead as it is hidden and disabled by default. As such, we can consider it experiemental and drop it without deprecation. PG mailing list discussiong: https://www.postgresql.org/message-id/CAPpHfducQ0U8noyb2L3VChsyBMsc5V2Ej2whmEuxmAgHa2jVXg%40mail.gmail.com --- app/models/user.rb | 18 ++++----- app/models/user_search.rb | 21 ---------- config/site_settings.yml | 3 -- ...ser_search_similar_results_site_setting.rb | 12 ++++++ ...240912061806_drop_trgm_indexes_on_users.rb | 13 ++++++ spec/models/user_search_spec.rb | 40 ------------------- 6 files changed, 33 insertions(+), 74 deletions(-) create mode 100644 db/migrate/20240912061702_drop_user_search_similar_results_site_setting.rb create mode 100644 db/migrate/20240912061806_drop_trgm_indexes_on_users.rb diff --git a/app/models/user.rb b/app/models/user.rb index 4b930f1ead3..29e47a444be 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -2291,14 +2291,12 @@ end # # Indexes # -# idx_users_admin (id) WHERE admin -# idx_users_moderator (id) WHERE moderator -# index_users_on_last_posted_at (last_posted_at) -# index_users_on_last_seen_at (last_seen_at) -# index_users_on_name_trgm (name) USING gist -# index_users_on_secure_identifier (secure_identifier) UNIQUE -# index_users_on_uploaded_avatar_id (uploaded_avatar_id) -# index_users_on_username (username) UNIQUE -# index_users_on_username_lower (username_lower) UNIQUE -# index_users_on_username_lower_trgm (username_lower) USING gist +# idx_users_admin (id) WHERE admin +# idx_users_moderator (id) WHERE moderator +# index_users_on_last_posted_at (last_posted_at) +# index_users_on_last_seen_at (last_seen_at) +# index_users_on_secure_identifier (secure_identifier) UNIQUE +# index_users_on_uploaded_avatar_id (uploaded_avatar_id) +# index_users_on_username (username) UNIQUE +# index_users_on_username_lower (username_lower) UNIQUE # diff --git a/app/models/user_search.rb b/app/models/user_search.rb index ba48c37ea5f..6c8f01286bf 100644 --- a/app/models/user_search.rb +++ b/app/models/user_search.rb @@ -172,27 +172,6 @@ class UserSearch .each { |id| users << id } end - return users.to_a if users.size >= @limit - - # 6. similar usernames / names - if @term.present? && SiteSetting.user_search_similar_results - if SiteSetting.enable_names? - scoped_users - .where("username_lower <-> ? < 1 OR name <-> ? < 1", @term, @term) - .order(["LEAST(username_lower <-> ?, name <-> ?) ASC", @term, @term]) - .limit(@limit - users.size) - .pluck(:id) - .each { |id| users << id } - else - scoped_users - .where("username_lower <-> ? < 1", @term) - .order(["username_lower <-> ? ASC", @term]) - .limit(@limit - users.size) - .pluck(:id) - .each { |id| users << id } - end - end - users.to_a end diff --git a/config/site_settings.yml b/config/site_settings.yml index ee2ece4ff26..ba3d67b24bb 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -2580,9 +2580,6 @@ backups: client: true search: - user_search_similar_results: - default: false - hidden: true prioritize_exact_search_title_match: default: true hidden: true diff --git a/db/migrate/20240912061702_drop_user_search_similar_results_site_setting.rb b/db/migrate/20240912061702_drop_user_search_similar_results_site_setting.rb new file mode 100644 index 00000000000..15f393cb519 --- /dev/null +++ b/db/migrate/20240912061702_drop_user_search_similar_results_site_setting.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true +class DropUserSearchSimilarResultsSiteSetting < ActiveRecord::Migration[7.1] + def up + execute <<~SQL + DELETE FROM site_settings WHERE name = 'user_search_similar_results'; + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/db/migrate/20240912061806_drop_trgm_indexes_on_users.rb b/db/migrate/20240912061806_drop_trgm_indexes_on_users.rb new file mode 100644 index 00000000000..f31b33a21f6 --- /dev/null +++ b/db/migrate/20240912061806_drop_trgm_indexes_on_users.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true +class DropTrgmIndexesOnUsers < ActiveRecord::Migration[7.1] + def up + execute <<~SQL + DROP INDEX IF EXISTS index_users_on_username_lower_trgm; + DROP INDEX IF EXISTS index_users_on_name_trgm; + SQL + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/spec/models/user_search_spec.rb b/spec/models/user_search_spec.rb index 073852d49ee..2d5bc67541a 100644 --- a/spec/models/user_search_spec.rb +++ b/spec/models/user_search_spec.rb @@ -279,44 +279,4 @@ RSpec.describe UserSearch do expect(results[2]).to eq("mrorange") end end - - context "when using SiteSetting.user_search_similar_results" do - it "should find the user even with a typo if the setting is enabled" do - rafael = Fabricate(:user, username: "rafael", name: "Rafael Silva") - codinghorror = Fabricate(:user, username: "codinghorror", name: "Jeff Atwood") - pfaffman = Fabricate(:user, username: "pfaffman") - zogstrip = Fabricate(:user, username: "zogstrip", name: "Régis Hanol") - roman = Fabricate(:user, username: "roman", name: "Roman Rizzi") - - SiteSetting.user_search_similar_results = false - expect(UserSearch.new("rafel").search).to be_blank - expect(UserSearch.new("codding").search).to be_blank - expect(UserSearch.new("pffman").search).to be_blank - - SiteSetting.user_search_similar_results = true - expect(UserSearch.new("rafel").search).to include(rafael) - expect(UserSearch.new("codding").search).to include(codinghorror) - expect(UserSearch.new("pffman").search).to include(pfaffman) - - SiteSetting.user_search_similar_results = false - expect(UserSearch.new("silvia").search).to be_blank - expect(UserSearch.new("atwod").search).to be_blank - expect(UserSearch.new("regis").search).to be_blank - expect(UserSearch.new("reg").search).to be_blank - - SiteSetting.user_search_similar_results = true - expect(UserSearch.new("silvia").search).to include(rafael) - expect(UserSearch.new("atwod").search).to include(codinghorror) - expect(UserSearch.new("regis").search).to include(zogstrip) - expect(UserSearch.new("reg").search).to include(zogstrip) - end - - it "orders the results by similarity" do - zogstrip = Fabricate(:user, username: "zogstrip", name: "Régis Hanol") - roman = Fabricate(:user, username: "roman", name: "Roman Rizzi") - SiteSetting.user_search_similar_results = true - - expect(UserSearch.new("regis").search.first).to eq(zogstrip) - end - end end