Better caching and indexing of broadcast messagesbroadcast-messages-cache

Caching of BroadcastMessage instances has been changed so a cache stays
valid as long as the default cache expiration time permits, instead of
the cache being expired after 1 minute. When modifying broadcast
messages the cache is flushed automatically.

To remove the need for performing sequence scans on the
"broadcast_messages" table we also add an index on (starts_at, ends_at,
id), permitting PostgreSQL to use an index scan to get all necessary
data.

Finally this commit adds a few NOT NULL constraints to the table to
match the Rails validations.

Fixes gitlab-org/gitlab-ce#31706

6 files changed, 81 insertions, 9 deletions

diff --git a/app/models/broadcast_message.rb b/app/models/broadcast_message.rb
index 944725d91c3..3692bcc680d 100644
--- a/app/models/broadcast_message.rb
+++ b/app/models/broadcast_message.rb
@@ -14,9 +14,15 @@ class BroadcastMessage < ActiveRecord::Base
   default_value_for :color, '#E75E40'
   default_value_for :font,  '#FFFFFF'
 
+  CACHE_KEY = 'broadcast_message_current'.freeze
+
+  after_commit :flush_redis_cache
+
   def self.current
-    Rails.cache.fetch("broadcast_message_current", expires_in: 1.minute) do
-      where('ends_at > :now AND starts_at <= :now', now: Time.zone.now).order([:created_at, :id]).to_a
+    Rails.cache.fetch(CACHE_KEY) do
+      where('ends_at > :now AND starts_at <= :now', now: Time.zone.now)
+        .reorder(id: :asc)
+        .to_a
     end
   end
 
@@ -31,4 +37,8 @@ class BroadcastMessage < ActiveRecord::Base
   def ended?
     ends_at < Time.zone.now
   end
+
+  def flush_redis_cache
+    Rails.cache.delete(CACHE_KEY)
+  end
 end
diff --git a/changelogs/unreleased/broadcast-messages-cache.yml b/changelogs/unreleased/broadcast-messages-cache.yml
new file mode 100644
index 00000000000..a3c9e1ff465
--- /dev/null
+++ b/changelogs/unreleased/broadcast-messages-cache.yml
@@ -0,0 +1,4 @@
+---
+title: Better caching and indexing of broadcast messages
+merge_request:
+author:
diff --git a/db/migrate/20170809133343_add_broadcast_messages_index.rb b/db/migrate/20170809133343_add_broadcast_messages_index.rb
new file mode 100644
index 00000000000..4ab2ddb059d
--- /dev/null
+++ b/db/migrate/20170809133343_add_broadcast_messages_index.rb
@@ -0,0 +1,21 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddBroadcastMessagesIndex < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  COLUMNS = %i[starts_at ends_at id].freeze
+
+  def up
+    add_concurrent_index :broadcast_messages, COLUMNS
+  end
+
+  def down
+    remove_concurrent_index :broadcast_messages, COLUMNS
+  end
+end
diff --git a/db/migrate/20170809134534_add_broadcast_message_not_null_constraints.rb b/db/migrate/20170809134534_add_broadcast_message_not_null_constraints.rb
new file mode 100644
index 00000000000..13e8ef52f22
--- /dev/null
+++ b/db/migrate/20170809134534_add_broadcast_message_not_null_constraints.rb
@@ -0,0 +1,17 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddBroadcastMessageNotNullConstraints < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  COLUMNS = %i[starts_at ends_at created_at updated_at message_html]
+
+  def change
+    COLUMNS.each do |column|
+      change_column_null :broadcast_messages, column, false
+    end
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index ed3cf70bcdd..032ab6ec15d 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170807160457) do
+ActiveRecord::Schema.define(version: 20170809134534) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -163,16 +163,18 @@ ActiveRecord::Schema.define(version: 20170807160457) do
 
   create_table "broadcast_messages", force: :cascade do |t|
     t.text "message", null: false
-    t.datetime "starts_at"
-    t.datetime "ends_at"
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime "starts_at", null: false
+    t.datetime "ends_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.string "color"
     t.string "font"
-    t.text "message_html"
+    t.text "message_html", null: false
     t.integer "cached_markdown_version"
   end
 
+  add_index "broadcast_messages", ["starts_at", "ends_at", "id"], name: "index_broadcast_messages_on_starts_at_and_ends_at_and_id", using: :btree
+
   create_table "chat_names", force: :cascade do |t|
     t.integer "user_id", null: false
     t.integer "service_id", null: false
diff --git a/spec/models/broadcast_message_spec.rb b/spec/models/broadcast_message_spec.rb
index a8ca1d110e4..3369aef1d3e 100644
--- a/spec/models/broadcast_message_spec.rb
+++ b/spec/models/broadcast_message_spec.rb
@@ -20,7 +20,7 @@ describe BroadcastMessage do
     it { is_expected.not_to allow_value('000').for(:font) }
   end
 
-  describe '.current' do
+  describe '.current', :use_clean_rails_memory_store_caching do
     it 'returns message if time match' do
       message = create(:broadcast_message)
 
@@ -45,6 +45,14 @@ describe BroadcastMessage do
 
       expect(described_class.current).to be_empty
     end
+
+    it 'caches the output of the query' do
+      create(:broadcast_message)
+
+      expect(described_class).to receive(:where).and_call_original.once
+
+      2.times { described_class.current }
+    end
   end
 
   describe '#active?' do
@@ -102,4 +110,14 @@ describe BroadcastMessage do
       end
     end
   end
+
+  describe '#flush_redis_cache' do
+    it 'flushes the Redis cache' do
+      message = create(:broadcast_message)
+
+      expect(Rails.cache).to receive(:delete).with(described_class::CACHE_KEY)
+
+      message.flush_redis_cache
+    end
+  end
 end