diff --git a/src/metabase/driver/sql_jdbc/connection.clj b/src/metabase/driver/sql_jdbc/connection.clj index 31558bf63acd2102832fe629379dcf58f5d6b48d..15946b39234936ac0b8ff82d106ae625471de42b 100644 --- a/src/metabase/driver/sql_jdbc/connection.clj +++ b/src/metabase/driver/sql_jdbc/connection.clj @@ -3,7 +3,6 @@ multimethods for SQL JDBC drivers." (:require [clojure.java.jdbc :as jdbc] - [metabase.config :as config] [metabase.connection-pool :as connection-pool] [metabase.db.connection :as mdb.connection] [metabase.driver :as driver] @@ -11,6 +10,8 @@ [metabase.lib.metadata.jvm :as lib.metadata.jvm] [metabase.models.database :refer [Database]] [metabase.models.interface :as mi] + [metabase.models.setting :as setting] + [metabase.query-processor.context.default :as context.default] [metabase.query-processor.store :as qp.store] [metabase.util :as u] [metabase.util.i18n :refer [trs tru]] @@ -81,6 +82,24 @@ :catalog) details)) +(setting/defsetting jdbc-data-warehouse-max-connection-pool-size + "Maximum size of the c3p0 connection pool." + :visibility :internal + :type :integer + :default 15) + +(setting/defsetting jdbc-data-warehouse-unreturned-connection-timeout-seconds + "Kill connections if they are unreturned after this amount of time. In theory this should not be needed because the QP + will kill connections that time out, but in practice it seems that connections disappear into the ether every once + in a while; rather than exhaust the connection pool, let's be extra safe. This should be the same as the query + timeout in [[metabase.query-processor.context.default/query-timeout-ms]] by default." + :visibility :internal + :type :integer + :getter (fn [] + (or (setting/get-value-of-type :integer :jdbc-data-warehouse-unreturned-connection-timeout-seconds) + (long (/ context.default/query-timeout-ms 1000)))) + :setter :none) + (defmethod data-warehouse-connection-pool-properties :default [driver database] { ;; only fetch one new connection at a time, rather than batching fetches (default = 3 at a time). This is done in @@ -90,8 +109,7 @@ "maxIdleTime" (* 3 60 60) ; 3 hours "minPoolSize" 1 "initialPoolSize" 1 - "maxPoolSize" (or (config/config-int :mb-jdbc-data-warehouse-max-connection-pool-size) - 15) + "maxPoolSize" (jdbc-data-warehouse-max-connection-pool-size) ;; [From dox] If true, an operation will be performed at every connection checkout to verify that the connection is ;; valid. [...] ;; Testing Connections in checkout is the simplest and most reliable form of Connection testing, ;; but for better performance, consider verifying connections periodically using `idleConnectionTestPeriod`. [...] @@ -116,6 +134,10 @@ ;; ;; Kill idle connections above the minPoolSize after 5 minutes. "maxIdleTimeExcessConnections" (* 5 60) + ;; kill connections after this amount of time if they haven't been returned -- this should be the same as the query + ;; timeout. This theoretically shouldn't happen since the QP should kill things after a certain timeout but it's + ;; better to be safe than sorry -- it seems like in practice some connections disappear into the ether + "unreturnedConnectionTimeout" (jdbc-data-warehouse-unreturned-connection-timeout-seconds) ;; Set the data source name so that the c3p0 JMX bean has a useful identifier, which incorporates the DB ID, driver, ;; and name from the details "dataSourceName" (format "db-%d-%s-%s" diff --git a/test/metabase/driver/sql_jdbc/connection_test.clj b/test/metabase/driver/sql_jdbc/connection_test.clj index b53d7571fcd605de188f6a1f1d15d7120446df4c..4edb14cf988d990d1762d800244e7c6718fd2e87 100644 --- a/test/metabase/driver/sql_jdbc/connection_test.clj +++ b/test/metabase/driver/sql_jdbc/connection_test.clj @@ -76,7 +76,7 @@ (testing "the pool has been destroyed" (is @destroyed?)))))))))) -(deftest c3p0-datasource-name-test +(deftest ^:parallel c3p0-datasource-name-test (mt/test-drivers (sql-jdbc.tu/sql-jdbc-drivers) (testing "The dataSourceName c3p0 property is set properly for a database" (let [db (mt/db) @@ -87,7 +87,7 @@ ;; ensure that, for any sql-jdbc driver anyway, we found *some* DB name to use in this String (is (not= db-nm "null")))))) -(deftest same-connection-details-result-in-equal-specs-test +(deftest ^:parallel same-connection-details-result-in-equal-specs-test (testing "Two JDBC specs created with the same details must be considered equal for the connection pool cache to work correctly" ;; this is only really a concern for drivers like Spark SQL that create custom DataSources instead of plain details ;; maps -- those DataSources need to be considered equal based on the connection string/properties @@ -201,3 +201,14 @@ (is (= first-pool second-pool)) (is (= ::audit-db-not-in-cache! (get @#'sql-jdbc.conn/database-id->connection-pool audit-db-id ::audit-db-not-in-cache!))))))) + +(deftest ^:parallel include-unreturned-connection-timeout-test + (testing "We should be setting unreturnedConnectionTimeout; it should be the same as the query timeout (#33646)" + (is (=? {"unreturnedConnectionTimeout" integer?} + (sql-jdbc.conn/data-warehouse-connection-pool-properties :h2 (mt/db)))))) + +(deftest unreturned-connection-timeout-test + (testing "We should be able to set jdbc-data-warehouse-unreturned-connection-timeout-seconds via env var (#33646)" + (mt/with-temp-env-var-value [mb-jdbc-data-warehouse-unreturned-connection-timeout-seconds "20"] + (is (= 20 + (sql-jdbc.conn/jdbc-data-warehouse-unreturned-connection-timeout-seconds))))))