From 462ab5241c5e0154ba096328ec25d375d73d8787 Mon Sep 17 00:00:00 2001 From: Vishnu Narayanan Date: Thu, 7 Aug 2025 15:59:40 +0530 Subject: [PATCH] perf: fix notifications duplicate query and add composite index (#12110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Database CPU utilization was spiking due to expensive notification COUNT queries. Analysis revealed two critical issues: 1. Missing database index: Notification count queries were performing table scans without proper indexing 2. Duplicate WHERE clauses: SQL queries contained redundant read_at IS NULL conditions, causing unnecessary query complexity ### Root Cause Analysis The expensive queries were: ``` -- 41.61 calls/sec with duplicate condition SELECT COUNT(*) FROM "notifications" WHERE "notifications"."user_id" = $1 AND "notifications"."account_id" = $2 AND "notifications"."snoozed_until" IS NULL AND "notifications"."read_at" IS NULL AND "notifications"."read_at" IS NULL -- Duplicate! ``` This was caused by a logic error in NotificationFinder#unread_count introduced in commit cd06b2b33 (PR #8907). The method assumed @notifications contained all notifications, but @notifications was already filtered to unread notifications in most cases. ### The Default Query Flow: 1. Frontend calls: NotificationsAPI.getUnreadCount() → /notifications/unread_count 2. No parameters sent, so params = {} 3. NotificationFinder setup: - find_all_notifications: WHERE user_id = ? AND account_id = ? - filter_snoozed_notifications: WHERE snoozed_until IS NULL - filter_read_notifications: WHERE read_at IS NULL (because type_included?('read') is false) 4. unread_count called: Adds another WHERE read_at IS NULL ---- ### Solution 1. Added Missing Database Index - Index: (user_id, account_id, snoozed_until, read_at) 2. Fixed Duplicate WHERE Clause Logic --- app/finders/notification_finder.rb | 12 ++++++++--- ...307_add_notifications_performance_index.rb | 11 ++++++++++ db/schema.rb | 3 ++- spec/finders/notification_finder_spec.rb | 21 +++++++++++++++++-- 4 files changed, 41 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20250805160307_add_notifications_performance_index.rb diff --git a/app/finders/notification_finder.rb b/app/finders/notification_finder.rb index ccfe470a0..e1958827a 100644 --- a/app/finders/notification_finder.rb +++ b/app/finders/notification_finder.rb @@ -15,7 +15,13 @@ class NotificationFinder end def unread_count - @notifications.where(read_at: nil).count + if type_included?('read') + # If we're including read notifications, filter to unread + @notifications.where(read_at: nil).count + else + # Already filtered to unread notifications, just count + @notifications.count + end end def count @@ -27,7 +33,7 @@ class NotificationFinder def set_up find_all_notifications filter_snoozed_notifications - fitler_read_notifications + filter_read_notifications end def find_all_notifications @@ -38,7 +44,7 @@ class NotificationFinder @notifications = @notifications.where(snoozed_until: nil) unless type_included?('snoozed') end - def fitler_read_notifications + def filter_read_notifications @notifications = @notifications.where(read_at: nil) unless type_included?('read') end diff --git a/db/migrate/20250805160307_add_notifications_performance_index.rb b/db/migrate/20250805160307_add_notifications_performance_index.rb new file mode 100644 index 000000000..6ab96a238 --- /dev/null +++ b/db/migrate/20250805160307_add_notifications_performance_index.rb @@ -0,0 +1,11 @@ +class AddNotificationsPerformanceIndex < ActiveRecord::Migration[7.1] + disable_ddl_transaction! + + def change + # Add composite index to optimize notification count queries + # This covers the common query pattern: WHERE user_id = ? AND account_id = ? AND snoozed_until IS NULL AND read_at IS NULL + add_index :notifications, [:user_id, :account_id, :snoozed_until, :read_at], + name: 'idx_notifications_performance', + algorithm: :concurrently + end +end diff --git a/db/schema.rb b/db/schema.rb index eb7409b8b..c94b3618a 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[7.1].define(version: 2025_07_22_152516) do +ActiveRecord::Schema[7.1].define(version: 2025_08_05_160307) do # These extensions should be enabled to support this database enable_extension "pg_stat_statements" enable_extension "pg_trgm" @@ -909,6 +909,7 @@ ActiveRecord::Schema[7.1].define(version: 2025_07_22_152516) do t.index ["last_activity_at"], name: "index_notifications_on_last_activity_at" t.index ["primary_actor_type", "primary_actor_id"], name: "uniq_primary_actor_per_account_notifications" t.index ["secondary_actor_type", "secondary_actor_id"], name: "uniq_secondary_actor_per_account_notifications" + t.index ["user_id", "account_id", "snoozed_until", "read_at"], name: "idx_notifications_performance" t.index ["user_id"], name: "index_notifications_on_user_id" end diff --git a/spec/finders/notification_finder_spec.rb b/spec/finders/notification_finder_spec.rb index 962edc297..882bfee2c 100644 --- a/spec/finders/notification_finder_spec.rb +++ b/spec/finders/notification_finder_spec.rb @@ -66,14 +66,31 @@ RSpec.describe NotificationFinder do expect(subject.unread_count).to eq(3) expect(subject.count).to eq(3) end + + it 'avoids duplicate filtering in unread_count method' do + # Test the logical fix: when no 'read' filter is included, + # @notifications is already filtered to unread, so unread_count + # should just count without adding another read_at filter + + allow(subject.instance_variable_get(:@notifications)).to receive(:where).and_call_original + allow(subject.instance_variable_get(:@notifications)).to receive(:count).and_call_original + + result = subject.unread_count + + # Should return correct count without additional where clause + expect(result).to eq(3) + + # The fix ensures that when params[:includes] doesn't contain 'read', + # unread_count uses @notifications.count instead of @notifications.where(read_at: nil).count + end end context 'with filters applied' do let(:params) { { includes: %w[read snoozed] } } it 'adjusts counts based on included statuses' do - expect(subject.unread_count).to eq(4) - expect(subject.count).to eq(6) + expect(subject.unread_count).to eq(4) # 3 unread + 1 snoozed (which is unread) + expect(subject.count).to eq(6) # all notifications including read and snoozed end end end