# HG changeset patch
# User Vincent Tondellier <tonton+hg@team1664.org>
# Date 1455477851 -3600
# Node ID f68abe1d7358d4ffea1b668265deed57dc808420
# Parent  716c0292967c16b0411c0d255babd21ead3910a5
Fix race condition in CrashGroup::find_or_create by using constraint and upsert (for pg > 9.5) or table lock

diff -r 716c0292967c -r f68abe1d7358 lib/CrashTest/Plugin/Storage/Sql/Model/CrashGroup.pm
--- a/lib/CrashTest/Plugin/Storage/Sql/Model/CrashGroup.pm	Sun Feb 14 20:18:39 2016 +0100
+++ b/lib/CrashTest/Plugin/Storage/Sql/Model/CrashGroup.pm	Sun Feb 14 20:24:11 2016 +0100
@@ -131,36 +131,101 @@
     return $results;
 }
 
+has signature_extractor => sub {
+    my $self = shift;
+    state $sig_extract = CrashTest::Plugin::CrashSignatureExtractor::C_Cpp->new(
+        app => $self->app,
+        config => $self->app->config->{Processor}->{CrashSignatureExtractor}->{C_Cpp},
+    );
+};
+
+sub _find_by_sig {
+    my ($self, $sig) = @_;
+
+    return $self->db->query(
+        "SELECT id, crash_thread_signature_bt <-> \$1 AS dist
+        FROM crash_groups
+        WHERE crash_thread_signature_bt % \$1
+        ORDER BY dist ASC
+        LIMIT 1",
+        $sig
+    )->hash;
+}
+
+sub _create_pg95 {
+    my ($self, $uuid, $sig, $title) = @_;
+
+    my $crash_group = $self->db->query(
+        "INSERT INTO crash_groups (uuid, crash_thread_signature_bt, title)
+        VALUES (?, ?, ?)
+        ON CONFLICT DO NOTHING
+        RETURNING id",
+        $uuid, $sig, $title
+    )->hash;
+
+    if(!defined($crash_group)) {
+        $crash_group = $self->_find_by_sig($sig);
+        warn "crash group is null after insert, reselected " . $crash_group->{id} . "\n";
+    }
+
+    return $crash_group;
+}
+
+sub _create_pg94 {
+    my ($self, $uuid, $sig, $title) = @_;
+
+    my $tx = $self->db->begin;
+
+    # avoid race condition, no other safe way without upsert
+    $self->db->query("LOCK TABLE crash_groups IN EXCLUSIVE MODE")->finish;
+
+    # this query is NOT concurrency safe, there is a possible write race. That's why we lock the table.
+    my $crash_group = $self->db->query(
+        "INSERT INTO crash_groups (uuid, crash_thread_signature_bt, title)
+        SELECT \$1, \$2, \$3
+        WHERE NOT EXISTS (SELECT 1 FROM crash_groups WHERE crash_thread_signature_bt % \$2)
+        RETURNING id",
+        $uuid, $sig, $title
+    )->hash;
+
+    # may happen if there is an insert between the select (in _find_by_sig) and the lock
+    if(!defined($crash_group)) {
+        $crash_group = $self->_find_by_sig($sig);
+        warn "crash group is null after insert, reselected " . $crash_group->{id} . "\n";
+    }
+
+    $tx->commit;
+
+    return $crash_group;
+}
+
+sub _set_similarity_limit {
+    my $self = shift;
+    my $max_dist = $self->app->config->{Processor}->{CrashSignatureExtractor}->{C_Cpp}->{GroupMaxDistance} || 0.1;
+    $self->db->query("SELECT set_limit(?)", 1.0 - $max_dist)->finish;
+}
 
 sub find_or_create {
     my ($self, $uuid, $pjson) = @_;
 
-    my $max_dist = $self->app->config->{Processor}->{CrashSignatureExtractor}->{C_Cpp}->{GroupMaxDistance} || 0.1;
-
-    my $sig_extract = CrashTest::Plugin::CrashSignatureExtractor::C_Cpp->new(
-        app => $self->app,
-        config => $self->app->config->{Processor}->{CrashSignatureExtractor}->{C_Cpp}
-    );
-
     my $raw_thread = dclone $pjson->{threads}->[$pjson->{crashing_thread}->{threads_index}];
     my $thread = CrashTest::Model::Thread->new($raw_thread);
 
-    my $sig = join "\n", @{$sig_extract->extract_signature($thread)};
+    my $sig = join "\n", @{$self->signature_extractor->extract_signature($thread)};
+
+    $self->_set_similarity_limit();
+    my $crash_group = $self->_find_by_sig($sig);
 
-    my $crash_group;
-    if($sig ne "") {
-        $crash_group = $self->db->query(
-            "SELECT id, crash_thread_signature_bt <-> ? AS dist FROM crash_groups ORDER BY dist LIMIT 1",
-            $sig
-        )->hash;
+    if(!$crash_group) {
+        if($sig ne "") {
+            my $title = $self->signature_extractor->extract_signature_title($thread);
 
-        # race condition here ...
-
-        if(!$crash_group || $crash_group->{dist} > $max_dist) {
-            $crash_group = $self->db->query(
-                "INSERT INTO crash_groups (uuid, crash_thread_signature_bt, title) VALUES (?, ?, ?) RETURNING id",
-                $uuid, $sig, $sig_extract->extract_signature_title($thread)
-            )->hash;
+            if($self->db->dbh->{pg_server_version} >= 90500) {
+                #warn "PostgreSQL 9.5, nice !";
+                $crash_group = $self->_create_pg95($uuid, $sig, $title);
+            } else {
+                $crash_group = $self->_create_pg94($uuid, $sig, $title);
+            }
         }
     }
     return $crash_group;
diff -r 716c0292967c -r f68abe1d7358 lib/CrashTest/Plugin/Storage/Sql/migrations_pg.sql
--- a/lib/CrashTest/Plugin/Storage/Sql/migrations_pg.sql	Sun Feb 14 20:18:39 2016 +0100
+++ b/lib/CrashTest/Plugin/Storage/Sql/migrations_pg.sql	Sun Feb 14 20:24:11 2016 +0100
@@ -79,20 +79,19 @@
 -- 2 up
 -- ###########################################################################
 
+DROP INDEX crash_report_datas_idx_extract_crashing_functions;
+SELECT set_limit(0.1);
 CREATE TABLE "crash_groups" (
     "id" serial NOT NULL,
     "uuid" uuid NOT NULL,
     "title" character varying NOT NULL,
     "crash_thread_signature_bt" text NOT NULL,
+
+    EXCLUDE USING gist (crash_thread_signature_bt gist_trgm_ops WITH %),
     CONSTRAINT "crash_groups_uuid_idx" UNIQUE ("uuid"),
     PRIMARY KEY ("id")
 );
 
-DROP INDEX crash_report_datas_idx_extract_crashing_functions;
-CREATE INDEX crash_groups_idx_crash_thread_signature_bt ON crash_groups USING gist (
-    crash_thread_signature_bt gist_trgm_ops
-);
-
 ALTER TABLE "crash_reports" ADD COLUMN crash_group_id integer;
 ALTER TABLE "crash_reports" ADD COLUMN crash_group_distance real;
 ALTER TABLE "crash_reports" ADD CONSTRAINT "crash_reports_fk_crash_group_id" FOREIGN KEY ("crash_group_id")