From 9e4de49b79fe5effe2747fb2b200c4e18a2b7de0 Mon Sep 17 00:00:00 2001 From: rhymes Date: Thu, 29 Apr 2021 17:37:59 +0200 Subject: [PATCH] [Hotfix] [Search 2.0] Optimize performance of listings and articles and fix bugs (#13577) * Add tsvector index on listings * Fix sorting order when fetching tag flares * Add published_at as a sorting condition for Homepage::ArticlesQuery * Re-added param needed by ES, this got lost somewhere down the line --- app/controllers/search_controller.rb | 1 + app/queries/homepage/articles_query.rb | 2 +- app/services/search/postgres/article.rb | 4 +-- ...index_on_searchable_columns_to_listings.rb | 35 +++++++++++++++++++ db/schema.rb | 3 +- spec/queries/homepage/articles_query_spec.rb | 24 ++++++++++--- 6 files changed, 61 insertions(+), 8 deletions(-) create mode 100644 db/migrate/20210429094116_add_tsvector_index_on_searchable_columns_to_listings.rb diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index c8bce729f..eeaa3c2eb 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -51,6 +51,7 @@ class SearchController < ApplicationController :search_fields, :sort_by, :sort_direction, + :tag, :user_id, { tag_names: [], diff --git a/app/queries/homepage/articles_query.rb b/app/queries/homepage/articles_query.rb index d56e151fa..eb8f62cb7 100644 --- a/app/queries/homepage/articles_query.rb +++ b/app/queries/homepage/articles_query.rb @@ -17,7 +17,7 @@ module Homepage ].freeze DEFAULT_PER_PAGE = 60 MAX_PER_PAGE = 100 - SORT_PARAMS = %i[hotness_score public_reactions_count].freeze + SORT_PARAMS = %i[hotness_score public_reactions_count published_at].freeze def self.call(...) new(...).call diff --git a/app/services/search/postgres/article.rb b/app/services/search/postgres/article.rb index 0fbc4e26c..6fa0b2b3e 100644 --- a/app/services/search/postgres/article.rb +++ b/app/services/search/postgres/article.rb @@ -15,10 +15,10 @@ module Search relation = relation.search_articles(term) if term.present? - tag_flares = Homepage::FetchTagFlares.call(relation) - relation = sort(relation, sort_by, sort_direction) + tag_flares = Homepage::FetchTagFlares.call(relation) + # including user and organization as the last step as they are not needed # by the query that fetches tag flares, they are only needed by the serializer relation = relation.includes(:user, :organization) diff --git a/db/migrate/20210429094116_add_tsvector_index_on_searchable_columns_to_listings.rb b/db/migrate/20210429094116_add_tsvector_index_on_searchable_columns_to_listings.rb new file mode 100644 index 000000000..442fe4f26 --- /dev/null +++ b/db/migrate/20210429094116_add_tsvector_index_on_searchable_columns_to_listings.rb @@ -0,0 +1,35 @@ +class AddTsvectorIndexOnSearchableColumnnsToListings < ActiveRecord::Migration[6.1] + disable_ddl_transaction! + + def up + return if index_name_exists?(:classified_listings, :index_classified_listings_on_search_fields_as_tsvector) + + query = <<-SQL + ( + to_tsvector('simple'::regconfig, COALESCE((body_markdown)::text, ''::text)) || + to_tsvector('simple'::regconfig, COALESCE((cached_tag_list)::text, ''::text)) || + to_tsvector('simple'::regconfig, COALESCE((location)::text, ''::text)) || + to_tsvector('simple'::regconfig, COALESCE((slug)::text, ''::text)) || + to_tsvector('simple'::regconfig, COALESCE((title)::text, ''::text)) + ) + SQL + + add_index( + :classified_listings, + query, + using: :gin, + name: :index_classified_listings_on_search_fields_as_tsvector, + algorithm: :concurrently, + ) + end + + def down + return unless index_name_exists?(:classified_listings, :index_classified_listings_on_search_fields_as_tsvector) + + remove_index( + :classified_listings, + name: :index_classified_listings_on_search_fields_as_tsvector, + algorithm: :concurrently, + ) + end +end diff --git a/db/schema.rb b/db/schema.rb index 7e8b6f81d..30fe57b66 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_04_28_190634) do +ActiveRecord::Schema.define(version: 2021_04_29_094116) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -343,6 +343,7 @@ ActiveRecord::Schema.define(version: 2021_04_28_190634) do t.string "title" t.datetime "updated_at", null: false t.bigint "user_id" + t.index "(((((to_tsvector('simple'::regconfig, COALESCE(body_markdown, ''::text)) || to_tsvector('simple'::regconfig, COALESCE((cached_tag_list)::text, ''::text))) || to_tsvector('simple'::regconfig, COALESCE((location)::text, ''::text))) || to_tsvector('simple'::regconfig, COALESCE((slug)::text, ''::text))) || to_tsvector('simple'::regconfig, COALESCE((title)::text, ''::text))))", name: "index_classified_listings_on_search_fields_as_tsvector", using: :gin t.index ["classified_listing_category_id"], name: "index_classified_listings_on_classified_listing_category_id" t.index ["organization_id"], name: "index_classified_listings_on_organization_id" t.index ["published"], name: "index_classified_listings_on_published" diff --git a/spec/queries/homepage/articles_query_spec.rb b/spec/queries/homepage/articles_query_spec.rb index 1c9643e2c..cf917e3cf 100644 --- a/spec/queries/homepage/articles_query_spec.rb +++ b/spec/queries/homepage/articles_query_spec.rb @@ -154,7 +154,7 @@ RSpec.describe Homepage::ArticlesQuery, type: :query do end describe "sorting" do - it "sorts by the hotness_score", :aggregate_failures do + it "sorts by hotness_score", :aggregate_failures do article1, article2 = create_list(:article, 2) article1.update_columns(hotness_score: 1) @@ -167,7 +167,7 @@ RSpec.describe Homepage::ArticlesQuery, type: :query do expect(result).to eq([article1.id, article2.id]) end - it "sorts by the public_reactions_count" do + it "sorts by public_reactions_count", :aggregate_failures do article1, article2 = create_list(:article, 2) article1.update_columns(public_reactions_count: 1) @@ -180,14 +180,30 @@ RSpec.describe Homepage::ArticlesQuery, type: :query do expect(result).to eq([article1.id, article2.id]) end + it "sorts by published_at", :aggregate_failures do + article1, article2 = create_list(:article, 2) + + article1.update_columns(published_at: 2.weeks.ago) + article2.update_columns(published_at: 1.week.ago) + + result = described_class.call(sort_by: :published_at, sort_direction: :desc).ids + expect(result).to eq([article2.id, article1.id]) + + result = described_class.call(sort_by: :published_at, sort_direction: :asc).ids + expect(result).to eq([article1.id, article2.id]) + end + it "does not sort by unknown parameters" do + allow(Article).to receive(:order) + article1, article2 = create_list(:article, 2) article1.update_columns(comments_count: 1) article2.update_columns(comments_count: 2) - expect(Article).not_to receive(:order) # rubocop:disable RSpec/MessageSpies - described_class.call(sort_by: :comments_count, sort_direction: :desc).ids + described_class.call(sort_by: :comments_count, sort_direction: :desc) + + expect(Article).not_to have_received(:order) end end end