FIX: Use PostgreSQL 'ON CONFLICT' to deal with race condition

FIX: Use PostgreSQL ‘ON CONFLICT’ to deal with race condition

On busy sites, concurrent requests to insert into post_timings can occur, which was dealt with using Ruby exceptions.

This moves the handling to PostgreSQL which makes it a bit faster, and prevents a spam of ERROR in the database logs.

diff --git a/app/models/post_timing.rb b/app/models/post_timing.rb
index 0de539a..e3d94ba 100644
--- a/app/models/post_timing.rb
+++ b/app/models/post_timing.rb
@@ -33,20 +33,12 @@ class PostTiming < ActiveRecord::Base
   end
 
   def self.record_new_timing(args)
-    begin
-      DB.exec("INSERT INTO post_timings (topic_id, user_id, post_number, msecs)
-                SELECT :topic_id, :user_id, :post_number, :msecs
-                WHERE NOT EXISTS(SELECT 1 FROM post_timings
-                                 WHERE topic_id = :topic_id
-                                  AND user_id = :user_id
-                                  AND post_number = :post_number)",
-             args)
-    rescue PG::UniqueViolation
-      # concurrency is hard, we are not running serialized so this can possibly
-      # still happen, if it happens we just don't care, its an invalid record anyway
-      return
-    end
-
+    DB.exec("INSERT INTO post_timings (topic_id, user_id, post_number, msecs)
+              SELECT :topic_id, :user_id, :post_number, :msecs
+              ON CONFLICT DO NOTHING",
+            args)
+    # concurrency is hard, we are not running serialized so this can possibly
+    # still happen, if it happens we just don't care, its an invalid record anyway
     Post.where(['topic_id = :topic_id and post_number = :post_number', args]).update_all 'reads = reads + 1'
     UserStat.where(user_id: args[:user_id]).update_all 'posts_read_count = posts_read_count + 1'
   end

GitHub sha: 526e76ce

1 Like

A simple benchmark testing WHERE NOT EXISTS vs ON CONFLICT DO NOTHING doesn’t presents any regressions:

                  user       system     total    real                                                                                          
WHERE NOT EXISTS  2.217602   1.382899   3.600501 ( 73.832179)                                                                          
ON CONFLICT       2.348744   1.404524   3.753268 ( 71.617430)                                                                               


                  user       system     total    real                                                                                          
WHERE NOT EXISTS  1.910139   1.474664   3.384803 ( 72.858568)                                                                          
ON CONFLICT       2.267879   1.466823   3.734702 ( 71.462166) 

That doesn’t even test the exception handling, so looks like a safe change.

1 Like