[Search 2.0] Optimize search reading list (#13275)

* Use trigger and tsvector column to speed up reading list search

* Add organization destroy spec and todo note

* Fix failing data update script due to not null constraint

* Remove the leading anchor in the trigger regexp

* Fix reading list specs

* Address feedback

Gemfile

@@ -41,6 +41,7 @@ gem "fog-aws", "~> 3.10" # 'fog' gem to support Amazon Web Services
 gem "front_matter_parser", "~> 1.0" # Parse a front matter from syntactically correct strings or files
 gem "gemoji", "~> 4.0.0.rc2" # Character information and metadata for standard and custom emoji
 gem "gibbon", "~> 3.4" # API wrapper for MailChimp's API
+gem "hairtrigger", "~> 0.2.24" # HairTrigger lets you create and manage database triggers in a concise, db-agnostic, Rails-y way.
 gem "honeybadger", "~> 4.8" # Used for tracking application errors
 gem "honeycomb-beeline", "~> 2.4.0" # Monitoring and Observability gem
 gem "html_truncator", "~> 0.4" # Truncate an HTML string properly

Gemfile.lock

@@ -340,6 +340,10 @@ GEM
       guard (~> 2.8)
       guard-compat (~> 1.0)
       multi_json (~> 1.8)
+    hairtrigger (0.2.24)
+      activerecord (>= 5.0, < 7)
+      ruby2ruby (~> 2.4)
+      ruby_parser (~> 3.10)
     hashdiff (1.0.1)
     hashie (4.1.0)
     heapy (0.2.0)
@@ -684,6 +688,11 @@ GEM
     ruby-vips (2.1.0)
       ffi (~> 1.12)
     ruby2_keywords (0.0.4)
+    ruby2ruby (2.4.4)
+      ruby_parser (~> 3.1)
+      sexp_processor (~> 4.6)
+    ruby_parser (3.15.1)
+      sexp_processor (~> 4.9)
     rubyzip (2.3.0)
     s3_direct_upload (0.1.7)
       coffee-rails (>= 3.1)
@@ -715,6 +724,7 @@ GEM
       childprocess (>= 0.5, < 4.0)
       rubyzip (>= 1.2.2)
     semantic_range (3.0.0)
+    sexp_processor (4.15.2)
     shellany (0.0.1)
     shoulda-matchers (4.5.1)
       activesupport (>= 4.2.0)
@@ -892,6 +902,7 @@ DEPENDENCIES
   gibbon (~> 3.4)
   guard (~> 2.16)
   guard-livereload (~> 2.5)
+  hairtrigger (~> 0.2.24)
   honeybadger (~> 4.8)
   honeycomb-beeline (~> 2.4.0)
   html_truncator (~> 0.4)

app/models/article.rb

@@ -114,18 +114,55 @@ class Article < ApplicationRecord
   after_commit :sync_related_elasticsearch_docs, on: %i[update]
   after_commit :remove_from_elasticsearch, on: [:destroy]
 
+  # The trigger `update_reading_list_document` is used to keep the `articles.reading_list_document` column updated.
+  #
+  # Its body is inserted in a PostgreSQL trigger function and that joins the columns values
+  # needed to search documents in the context of a "reading list".
+  #
+  # Please refer to https://github.com/jenseng/hair_trigger#usage in case you want to change or update the trigger.
+  #
+  # Additional information on how triggers work can be found in
+  # => https://www.postgresql.org/docs/11/trigger-definition.html
+  # => https://www.cybertec-postgresql.com/en/postgresql-how-to-write-a-trigger/
+  #
+  # Adapted from https://dba.stackexchange.com/a/289361/226575
+  trigger
+    .name(:update_reading_list_document).before(:insert, :update).for_each(:row)
+    .declare("l_org_vector tsvector; l_user_vector tsvector") do
+    <<~SQL
+      NEW.reading_list_document :=
+        to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) ||
+        to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) ||
+        to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) ||
+        to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) ||
+        to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) ||
+        to_tsvector('simple'::regconfig,
+          unaccent(
+            coalesce(
+              array_to_string(
+                -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute
+                regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'),
+                ' '
+              ),
+              ''
+            )
+          )
+        );
+    SQL
+  end
+
   serialize :cached_user
   serialize :cached_organization
 
-  # [@rhymes] this is adapted from the `search_fields` property in
-  # `config/elasticsearch/mappings/feed_content.json`
   pg_search_scope :search_reading_list,
-                  against: %i[body_markdown title cached_tag_list],
-                  associated_against: {
-                    organization: %i[name],
-                    user: %i[name username]
+                  against: :reading_list_document,
+                  using: {
+                    tsearch: {
+                      prefix: true,
+                      tsvector_column: :reading_list_document
+                    }
                   },
-                  using: { tsearch: { prefix: true } }
+                  ignoring: :accents
 
   # [@jgaskins] We use an index on `published`, but since it's a boolean value
   #   the Postgres query planner often skips it due to lack of diversity of the

app/models/organization.rb

@@ -14,6 +14,9 @@ class Organization < ApplicationRecord
   before_validation :downcase_slug
   before_validation :check_for_slug_change
   before_validation :evaluate_markdown
+
+  # TODO: [@rhymes] revisit this callback, `update_articles_cached_organization` and `article_sync`
+  # when we remove Elasticsearch
   before_save :update_articles
   before_save :remove_at_from_usernames
   before_save :generate_secret
@@ -22,6 +25,14 @@ class Organization < ApplicationRecord
   # https://guides.rubyonrails.org/active_record_callbacks.html#destroying-an-object
   before_destroy :cache_article_ids
 
+  after_save :bust_cache
+
+  # This callback will eventually invoke Article.update_cached_user to update the organization.name
+  # only when it has been changed, thus invoking the trigger on Article.reading_list_document
+  after_update_commit :update_articles_cached_organization
+  after_update_commit :sync_related_elasticsearch_docs
+  after_destroy_commit :bust_cache, :article_sync
+
   has_many :articles, dependent: :nullify
   has_many :collections, dependent: :nullify
   has_many :credits, dependent: :restrict_with_error
@@ -63,11 +74,6 @@ class Organization < ApplicationRecord
 
   validate :unique_slug_including_users_and_podcasts, if: :slug_changed?
 
-  after_save :bust_cache
-
-  after_commit :sync_related_elasticsearch_docs, on: :update
-  after_commit :bust_cache, :article_sync, on: :destroy
-
   mount_uploader :profile_image, ProfileImageUploader
   mount_uploader :nav_image, ProfileImageUploader
   mount_uploader :dark_nav_image, ProfileImageUploader
@@ -146,6 +152,12 @@ class Organization < ApplicationRecord
     articles.update(cached_organization: Articles::CachedEntity.from_object(self))
   end
 
+  def update_articles_cached_organization
+    return unless saved_change_to_attribute?(:name)
+
+    articles.update(cached_organization: Articles::CachedEntity.from_object(self))
+  end
+
   def bust_cache
     Organizations::BustCacheWorker.perform_async(id, slug)
   end

app/services/search/postgres/reading_list.rb

@@ -12,6 +12,7 @@ module Search
         "reactions.id AS reaction_id",
         "reactions.user_id AS reaction_user_id",
       ].freeze
+      REACTION_ATTRIBUTES = %i[id reactable_id user_id].freeze
       USER_ATTRIBUTES = %i[id name profile_image username].freeze
 
       DEFAULT_STATUSES = %w[confirmed valid].freeze
@@ -31,7 +32,7 @@ module Search
         per_page = [(per_page || DEFAULT_PER_PAGE).to_i, MAX_PER_PAGE].min
 
         result = find_articles(
-          user_id: user.id,
+          user: user,
           term: term,
           statuses: statuses,
           tags: tags,
@@ -58,12 +59,20 @@ module Search
         }
       end
 
-      def self.find_articles(user_id:, term:, statuses:, tags:, page:, per_page:)
-        relation = ::Article
-          .joins(:reactions)
-          .where("reactions.category": :readinglist)
-          .where("reactions.user_id": user_id)
-          .where("reactions.status": statuses)
+      def self.find_articles(user:, term:, statuses:, tags:, page:, per_page:)
+        # [@jgaskins, @rhymes] as `reactions` is potentially a big table, adding pagination
+        # to an INNER JOIN (eg. `joins(:reactions)`) exponentially decreases the performance,
+        # incrementing query time as the database has to scan all the rows just to discard
+        # them right after if they lie outside the bounds of the `OFFSET`.
+        # Even though it should have had a similar performance, we realized that a subquery
+        # enabled PostgreSQL query planner to drastically decrease the planned time (ca. 145x)
+        reaction_query_sql = user.reactions.readinglist
+          .where(status: statuses, reactable_type: "Article")
+          .order(created_at: :desc)
+          .select(*REACTION_ATTRIBUTES)
+          .to_sql
+
+        relation = Article.joins("INNER JOIN (#{reaction_query_sql}) reactions ON reactions.reactable_id = articles.id")
 
         relation = relation.search_reading_list(term) if term.present?
 
@@ -93,7 +102,7 @@ module Search
         # because we need to fetch the total number of articles, pre pagination
         total = relation.count
 
-        relation = relation.select(*ATTRIBUTES).order("reactions.created_at": :desc)
+        relation = relation.select(*ATTRIBUTES)
         relation = relation.page(page).per(per_page)
 
         {

db/migrate/20210405143721_add_reading_list_document_to_articles.rb

@@ -0,0 +1,6 @@
+class AddReadingListDocumentToArticles < ActiveRecord::Migration[6.0]
+  def change
+    add_column :articles, :reading_list_document, :tsvector
+  end
+end
+

db/migrate/20210405145104_add_index_to_reading_list_document_on_articles.rb

@@ -0,0 +1,7 @@
+class AddIndexToReadingListDocumentOnArticles < ActiveRecord::Migration[6.0]
+  disable_ddl_transaction!
+
+  def change
+    add_index :articles, :reading_list_document, using: "gin", algorithm: :concurrently
+  end
+end

db/migrate/20210407172628_create_trigger_articles_insert_update.rb

@@ -0,0 +1,39 @@
+# This migration was auto-generated via `rake db:generate_trigger_migration'.
+# While you can edit this file, any changes you make to the definitions here
+# will be undone by the next auto-generated trigger migration.
+
+class CreateTriggerArticlesInsertUpdate < ActiveRecord::Migration[6.1]
+  def up
+    create_trigger("update_reading_list_document", :generated => true, :compatibility => 1).
+        on("articles").
+        name("update_reading_list_document").
+        before(:insert, :update).
+        for_each(:row).
+        declare("l_org_vector tsvector; l_user_vector tsvector") do
+      <<-SQL_ACTIONS
+NEW.reading_list_document :=
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) ||
+  to_tsvector('simple'::regconfig,
+    unaccent(
+      coalesce(
+        array_to_string(
+          -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute
+          regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'),
+          ' '
+        ),
+        ''
+      )
+    )
+  );
+      SQL_ACTIONS
+    end
+  end
+
+  def down
+    drop_trigger("update_reading_list_document", "articles", :generated => true)
+  end
+end

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_03_31_181505) do
+ActiveRecord::Schema.define(version: 2021_04_07_172628) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "citext"
@@ -131,6 +131,7 @@ ActiveRecord::Schema.define(version: 2021_03_31_181505) do
     t.boolean "published_from_feed", default: false
     t.integer "rating_votes_count", default: 0, null: false
     t.integer "reactions_count", default: 0, null: false
+    t.tsvector "reading_list_document"
     t.integer "reading_time", default: 0
     t.boolean "receive_notifications", default: true
     t.integer "score", default: 0
@@ -164,6 +165,7 @@ ActiveRecord::Schema.define(version: 2021_03_31_181505) do
     t.index ["public_reactions_count"], name: "index_articles_on_public_reactions_count", order: :desc
     t.index ["published"], name: "index_articles_on_published"
     t.index ["published_at"], name: "index_articles_on_published_at"
+    t.index ["reading_list_document"], name: "index_articles_on_reading_list_document", using: :gin
     t.index ["slug", "user_id"], name: "index_articles_on_slug_and_user_id", unique: true
     t.index ["user_id"], name: "index_articles_on_user_id"
   end
@@ -1537,4 +1539,32 @@ ActiveRecord::Schema.define(version: 2021_03_31_181505) do
   add_foreign_key "users_settings", "users"
   add_foreign_key "webhook_endpoints", "oauth_applications"
   add_foreign_key "webhook_endpoints", "users"
+  create_trigger("update_reading_list_document", :generated => true, :compatibility => 1).
+      on("articles").
+      name("update_reading_list_document").
+      before(:insert, :update).
+      for_each(:row).
+      declare("l_org_vector tsvector; l_user_vector tsvector") do
+    <<-SQL_ACTIONS
+NEW.reading_list_document :=
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.body_markdown, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_tag_list, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_name, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.cached_user_username, ''))) ||
+  to_tsvector('simple'::regconfig, unaccent(coalesce(NEW.title, ''))) ||
+  to_tsvector('simple'::regconfig,
+    unaccent(
+      coalesce(
+        array_to_string(
+          -- cached_organization is serialized to the DB as a YAML string, we extract only the name attribute
+          regexp_match(NEW.cached_organization, 'name: (.*)$', 'n'),
+          ' '
+        ),
+        ''
+      )
+    )
+  );
+    SQL_ACTIONS
+  end
+
 end

lib/data_update_scripts/20210405152916_invoke_update_reading_list_document_trigger_on_articles.rb

@@ -0,0 +1,11 @@
+module DataUpdateScripts
+  class InvokeUpdateReadingListDocumentTriggerOnArticles
+    def run
+      return unless ActiveRecord::Base.connection.column_exists?(:articles, :reading_list_document)
+
+      # by updating `reading_list_document` to `NULL`,
+      # we invoke the `update_reading_list_document` trigger, bypassing Rails callbacks
+      Article.in_batches.update_all(reading_list_document: nil)
+    end
+  end
+end

spec/models/article_spec.rb

@@ -997,6 +997,44 @@ RSpec.describe Article, type: :model do
     end
   end
 
+  context "when triggers are invoked" do
+    let(:article) { create(:article) }
+
+    before do
+      article.update(body_markdown: "An intense movie")
+    end
+
+    it "sets .reading_list_document on insert" do
+      expect(article.reload.reading_list_document).to be_present
+    end
+
+    it "updates .reading_list_document with body_markdown" do
+      article.update(body_markdown: "Something has changed")
+
+      expect(article.reload.reading_list_document).to include("something")
+    end
+
+    it "updates .reading_list_document with cached_tag_list" do
+      article.update(tag_list: %w[rust python])
+
+      expect(article.reload.reading_list_document).to include("rust")
+    end
+
+    it "updates .reading_list_document with title" do
+      article.update(title: "Synecdoche, Los Angeles")
+
+      expect(article.reload.reading_list_document).to include("angeles")
+    end
+
+    it "removes a previous value from .reading_list_document on update", :aggregate_failures do
+      tag = article.tags.first.name
+      article.update(tag_list: %w[fsharp go])
+
+      expect(article.reload.reading_list_document).not_to include(tag)
+      expect(article.reload.reading_list_document).to include("fsharp")
+    end
+  end
+
   describe ".feed" do
     it "returns records with a subset of attributes" do
       feed_article = described_class.feed.first

spec/models/organization_spec.rb

@@ -254,7 +254,9 @@ RSpec.describe Organization, type: :model do
     context "when dealing with organization articles" do
       before do
         create(:organization_membership, user: user, organization: organization, type_of_user: "admin")
-        create(:article, organization: organization, user: user)
+        article = create(:article, organization: organization, user: user)
+        organization.articles << article
+        organization.save
       end
 
       it "updates the paths of the organization's articles" do
@@ -278,6 +280,39 @@ RSpec.describe Organization, type: :model do
         article = Article.find_by(organization_id: organization.id)
         expect(article.cached_organization.slug).to eq(new_slug)
       end
+
+      # these tests rely on `Organization.update_articles_cached_organization` callback,
+      # which will eventually invoke the trigger
+      # rubocop:disable RSpec/NestedGroups
+      context "when callbacks eventually invoke the trigger on Article.reading_list_document" do
+        it "updates the articles .reading_list_document when updating the name" do
+          article = Article.find_by(organization_id: organization.id)
+          old_reading_list_document = article.reading_list_document
+
+          organization.update(name: "ACME Org")
+
+          expect(article.reload.reading_list_document).not_to eq(old_reading_list_document)
+        end
+
+        it "does not update the articles .reading_list_document when updating the company_size" do
+          article = Article.find_by(organization_id: organization.id)
+          old_reading_list_document = article.reading_list_document
+
+          organization.update(company_size: "200")
+
+          expect(article.reload.reading_list_document).to eq(old_reading_list_document)
+        end
+
+        it "removes the organization name from the .reading_list_document after destroy" do
+          article = Article.find_by(organization_id: organization.id)
+
+          organization.update(name: "ACME")
+          organization.destroy
+
+          expect(article.reload.reading_list_document).not_to include("acme")
+        end
+      end
+      # rubocop:enable RSpec/NestedGroups
     end
   end
 

spec/services/search/postgres/reading_list_spec.rb

@@ -231,24 +231,18 @@ RSpec.describe Search::Postgres::ReadingList, type: :service do
       end
 
       it "matches against the article's user's name", :aggregate_failures do
-        article_user = article.user
-        article_user.update_columns(name: "Friday Sunday")
-
-        result = described_class.search_documents(user, term: "Frida")
+        result = described_class.search_documents(user, term: article.user_name.first(3))
         expect(extract_from_results(result, :path)).to include(article.path)
 
-        result = described_class.search_documents(user, term: "Sat")
+        result = described_class.search_documents(user, term: "notaname")
         expect(extract_from_results(result, :path)).to be_empty
       end
 
       it "matches against the article's user's username", :aggregate_failures do
-        article_user = article.user
-        article_user.update_columns(username: "fridaysunday")
-
-        result = described_class.search_documents(user, term: "Frida")
+        result = described_class.search_documents(user, term: article.user_username.first(3))
         expect(extract_from_results(result, :path)).to include(article.path)
 
-        result = described_class.search_documents(user, term: "Sat")
+        result = described_class.search_documents(user, term: "notausername")
         expect(extract_from_results(result, :path)).to be_empty
       end
     end