Commit a5c8a527 authored by Yorick Peterse's avatar Yorick Peterse

Better caching and indexing of broadcast messages

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
parent e80a893f
...@@ -14,9 +14,15 @@ class BroadcastMessage < ActiveRecord::Base ...@@ -14,9 +14,15 @@ class BroadcastMessage < ActiveRecord::Base
default_value_for :color, '#E75E40' default_value_for :color, '#E75E40'
default_value_for :font, '#FFFFFF' default_value_for :font, '#FFFFFF'
CACHE_KEY = 'broadcast_message_current'.freeze
after_commit :flush_redis_cache
def self.current def self.current
Rails.cache.fetch("broadcast_message_current", expires_in: 1.minute) do Rails.cache.fetch(CACHE_KEY) do
where('ends_at > :now AND starts_at <= :now', now: Time.zone.now).order([:created_at, :id]).to_a where('ends_at > :now AND starts_at <= :now', now: Time.zone.now)
.reorder(id: :asc)
.to_a
end end
end end
...@@ -31,4 +37,8 @@ class BroadcastMessage < ActiveRecord::Base ...@@ -31,4 +37,8 @@ class BroadcastMessage < ActiveRecord::Base
def ended? def ended?
ends_at < Time.zone.now ends_at < Time.zone.now
end end
def flush_redis_cache
Rails.cache.delete(CACHE_KEY)
end
end end
---
title: Better caching and indexing of broadcast messages
merge_request:
author:
# 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
# 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
...@@ -163,16 +163,18 @@ ActiveRecord::Schema.define(version: 20170809142252) do ...@@ -163,16 +163,18 @@ ActiveRecord::Schema.define(version: 20170809142252) do
create_table "broadcast_messages", force: :cascade do |t| create_table "broadcast_messages", force: :cascade do |t|
t.text "message", null: false t.text "message", null: false
t.datetime "starts_at" t.datetime "starts_at", null: false
t.datetime "ends_at" t.datetime "ends_at", null: false
t.datetime "created_at" t.datetime "created_at", null: false
t.datetime "updated_at" t.datetime "updated_at", null: false
t.string "color" t.string "color"
t.string "font" t.string "font"
t.text "message_html" t.text "message_html", null: false
t.integer "cached_markdown_version" t.integer "cached_markdown_version"
end 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| create_table "chat_names", force: :cascade do |t|
t.integer "user_id", null: false t.integer "user_id", null: false
t.integer "service_id", null: false t.integer "service_id", null: false
......
...@@ -20,7 +20,7 @@ describe BroadcastMessage do ...@@ -20,7 +20,7 @@ describe BroadcastMessage do
it { is_expected.not_to allow_value('000').for(:font) } it { is_expected.not_to allow_value('000').for(:font) }
end end
describe '.current' do describe '.current', :use_clean_rails_memory_store_caching do
it 'returns message if time match' do it 'returns message if time match' do
message = create(:broadcast_message) message = create(:broadcast_message)
...@@ -45,6 +45,14 @@ describe BroadcastMessage do ...@@ -45,6 +45,14 @@ describe BroadcastMessage do
expect(described_class.current).to be_empty expect(described_class.current).to be_empty
end 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 end
describe '#active?' do describe '#active?' do
...@@ -102,4 +110,14 @@ describe BroadcastMessage do ...@@ -102,4 +110,14 @@ describe BroadcastMessage do
end end
end 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 end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment