From 1dceed32b81007b372d4854ee8b8178b59d79fe2 Mon Sep 17 00:00:00 2001
From: Cam Saul <1455846+camsaul@users.noreply.github.com>
Date: Thu, 20 Jun 2024 18:44:34 -0700
Subject: [PATCH] Make the App DB transaction isolation level READ_COMMITTED
for MySQL [MEGA PERFORMANCE BOOST] (#44505)
* Make the App DB transaction isolation level READ_COMMITTED for MySQL/MariaDB [MEGA PERFORMANCE BOOST]
* Add test
---
src/metabase/db/data_source.clj | 11 ++++++++---
test/metabase/db/connection_test.clj | 7 +++++++
test/metabase/db/data_source_test.clj | 10 +++++-----
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/metabase/db/data_source.clj b/src/metabase/db/data_source.clj
index 622ad236cb9..6b781adf5cc 100644
--- a/src/metabase/db/data_source.clj
+++ b/src/metabase/db/data_source.clj
@@ -31,9 +31,14 @@
javax.sql.DataSource
(getConnection [_]
- (if properties
- (DriverManager/getConnection url properties)
- (DriverManager/getConnection url)))
+ (doto (if properties
+ (DriverManager/getConnection url properties)
+ (DriverManager/getConnection url))
+ ;; MySQL/MariaDB default to REPEATABLE_READ which ends up making everything SLOW because it locks all the time.
+ ;; Postgres defaults to READ_COMMITTED. Explicitly set transaction isolation for new connections so we can make
+ ;; sure we're using READ_COMMITTED. See https://metaboat.slack.com/archives/C04DN5VRQM6/p1718912820432359 for more
+ ;; info.
+ (.setTransactionIsolation java.sql.Connection/TRANSACTION_READ_COMMITTED)))
;; we don't use (.getConnection this url user password) so we don't need to implement it.
(getConnection [_ _user _password]
diff --git a/test/metabase/db/connection_test.clj b/test/metabase/db/connection_test.clj
index 19b7d45d18b..1e8ff8b7a58 100644
--- a/test/metabase/db/connection_test.clj
+++ b/test/metabase/db/connection_test.clj
@@ -1,6 +1,7 @@
(ns metabase.db.connection-test
(:require
[clojure.test :refer :all]
+ [metabase.db.connection :as mdb.connection]
[metabase.test :as mt]
[metabase.test.fixtures :as fixtures]
[toucan2.core :as t2]
@@ -131,3 +132,9 @@
(catch Exception e
(when-not (is-transaction-exception? e)
(throw e))))))))
+
+(deftest ^:parallel transaction-isolation-level-test
+ (testing "We should always use READ_COMMITTED for the app DB (#44505)"
+ (with-open [conn (.getConnection mdb.connection/*application-db*)]
+ (is (= java.sql.Connection/TRANSACTION_READ_COMMITTED
+ (.getTransactionIsolation conn))))))
diff --git a/test/metabase/db/data_source_test.clj b/test/metabase/db/data_source_test.clj
index 97209fd8b22..7a54dccb2ba 100644
--- a/test/metabase/db/data_source_test.clj
+++ b/test/metabase/db/data_source_test.clj
@@ -12,7 +12,7 @@
(defn- ->DataSource [s properties]
(#'mdb.data-source/->DataSource s (some-> (not-empty properties) connection-pool/map->properties)))
-(deftest broken-out-details-test
+(deftest ^:parallel broken-out-details-test
(testing :postgres
(is (= (->DataSource
"jdbc:postgresql://localhost:5432/metabase"
@@ -51,7 +51,7 @@
(is (= [{:one 1}]
(jdbc/query {:connection conn} "SELECT 1 AS one;")))))))
-(deftest connection-string-test
+(deftest ^:parallel connection-string-test
(let [data-source (mdb.data-source/raw-connection-string->DataSource
(format "jdbc:h2:mem:%s" (mt/random-name)))]
(with-open [conn (.getConnection data-source)]
@@ -72,7 +72,7 @@
(is (= (mdb.data-source/raw-connection-string->DataSource "jdbc:postgresql://localhost:5432/metabase")
(mdb.data-source/raw-connection-string->DataSource "postgres://localhost:5432/metabase")))))
-(deftest wonky-connection-string-test
+(deftest ^:parallel wonky-connection-string-test
(testing "Should handle malformed user:password@host:port strings (#14678, #20121)"
(doseq [subprotocol ["postgresql" "mysql"]]
(testing "user AND password"
@@ -96,7 +96,7 @@
{"user" "cam"})
(mdb.data-source/raw-connection-string->DataSource (str subprotocol "://cam@localhost/metabase?password=1234")))))))))
-(deftest raw-connection-string-with-separate-username-and-password-test
+(deftest ^:parallel raw-connection-string-with-separate-username-and-password-test
(testing "Raw connection string should support separate username and/or password (#20122)"
(testing "username and password"
(is (= (->DataSource
@@ -116,7 +116,7 @@
(mdb.data-source/raw-connection-string->DataSource "postgres://metabase" nil "1234")
(mdb.data-source/raw-connection-string->DataSource "postgres://metabase" "" "1234"))))))
-(deftest equality-test
+(deftest ^:parallel equality-test
(testing "Two DataSources with the same URL should be equal"
(is (= (mdb.data-source/raw-connection-string->DataSource "ABCD")
(mdb.data-source/raw-connection-string->DataSource "ABCD"))))
--
GitLab