-
- Downloads
Cherry pick bug fixes from master into release-0.36.x (oom fix and postgres...
Cherry pick bug fixes from master into release-0.36.x (oom fix and postgres sync with ssh tunnelling) (#13420)
* Half of fix for OOM in sync (#13288) (2.1) (#13325)
* Half of fix for OOM in sync (#13288) (2.1)
* truncate type/Text fields to 1234 (a distinct number we can use as a
heuristic for longer fields if desired)
* test truncation in metadata-queries-test
* test that fingerprints reflect these truncations in fingerprint-test
* Check database_type of text for substring of summaries #13288
in pg, many types go to :type/Text which cannot have substring on
them, such as xml, json, jsonb, etc.
* Skip fingerprinting "structured" fields, take substring of text fields
* "Sniff" JSON results when truncated in fingerprinting
- had to move the truncation-size var due to circularity, but it most
logically belongs in the fingerprinting namespace as that's the only
mechanism that really should be aware of it anyways
- get rid of :refer :all in a test ns
- fingerprinting doesn't happen on base_type :type/Structured so add
that to the query for fields to fingerprint
* Move `table-rows-sample` truncation option into options map
* Switch json fingerprinting entirely to heuristic rather than parsing
- faster in all cases and accuracy is not a particular goal
- doesn't rely on the truncation size for switching to heuristic
* Derive SerializedJson and XML from :type/Structured
* Fixes metadata queries test when no truncation-size provided
* JSON heuristic for array of booleans
- want to validate as JSON [true, false] but not [random tag]
* Removes two unused deps from fingerprinters
we no longer actually parse json so don't need cheshire and don't use
clojure.string/starts-with in favor of a little descent parser
* CamelCases schema name and move truncation-size to appropriate ns
truncation size used to be used in the fingerprinting analysis when
checking text for JSON (using a heuristic if it was truncated) and
passed into the query to get table summaries. Now its only passed into
the summaries query and the JSON recognizer in the text fingerprinter
always uses heuristics
* Fix test now that its not relying on truncation limit for heuristics
previously was making a string that was the `truncation-limit` length
to trigger the heuristics of json parsing. now we always use heuristic
so no need to construct such a precise length string.
* Fixes #8396 "Postgres sync not respecting SSH tunneling" (#13391)
* Use db pool connection rather than raw details
connection pool is correctly setup with ssh tunneling information (if
any)
* Don't error when getting postgres enum types
this was throwing errors when we allowed our pooled connections to
close underneath us. But this particular step just isn't important
enough for that. If we don't have a connection available to us, let
the more robust query stuff fail it rather than this one off
* Set idleConnectionTestPeriod to 120 when ssh tunneling
The ssh tunnelling apparatus needs some work. The tunnels are created
and put in the spec details map but then this is tossed away when we
create the pool.
```clojure
(defn- create-pool!
"Create a new C3P0 `ComboPooledDataSource` for connecting to the given `database`."
[{:keys [id details], driver :engine, :as database}]
{:pre [(map? database)]}
(log/debug (u/format-color 'cyan (trs "Creating new connection pool for {0} database {1} ..." driver id)))
(let [details-with-tunnel (ssh/include-ssh-tunnel details) ;; If the tunnel is disabled this returned unchanged
spec (connection-details->spec driver details-with-tunnel)
properties (data-warehouse-connection-pool-properties driver)]
(connection-pool/connection-pool-spec spec (merge properties
(when (ssh/use-ssh-tunnel? details)
{"idleConnectionTestPeriod" 120})))))
```
this is swapped into an atom of {db_id -> pooleddatasource} but this
means we don't have the :tunnel-session and :tunnel-tracker info from
`start-ssh-tunnel`.
* Remove connection pool keys when set to nil
our connection pool would end up with {db_id nil ...} for no
reason. Add tests to ensure.
This change is largely driven by the codox testing
requirement. There's no good way to test that the ssh tunneling
options includes
(when (ssh/use-ssh-tunnel? details) {"idleConnectionTestPeriod" 120})
and the test coverage checker was unhappy
* Correct ns form in test file
* Remove try/catch around enum types
there's already error handling around this sync step so let it
happen. The issue was that this check didn't use the correct
connection to respect tunnelling and therefore would throw an error in
getting enum types and fail the database sync step. That is now done
correctly so if this step fails the other steps would as well and we
no longer need to handle an error here specifically
* Move ssh heartbeat into ssh.clj and not use jdbc
jdbc had a way to test idle connections but other non-jdbc datasources
obviously wouldn't benefit. Luckily our ssh connection can handle this
on its own
Co-authored-by:
dpsutton <dan@dpsutton.com>
Showing
- src/metabase/db/metadata_queries.clj 26 additions, 11 deletionssrc/metabase/db/metadata_queries.clj
- src/metabase/driver/postgres.clj 16 additions, 14 deletionssrc/metabase/driver/postgres.clj
- src/metabase/driver/sql_jdbc/connection.clj 3 additions, 1 deletionsrc/metabase/driver/sql_jdbc/connection.clj
- src/metabase/sync/analyze/fingerprint.clj 8 additions, 1 deletionsrc/metabase/sync/analyze/fingerprint.clj
- src/metabase/sync/analyze/fingerprint/fingerprinters.clj 12 additions, 3 deletionssrc/metabase/sync/analyze/fingerprint/fingerprinters.clj
- src/metabase/types.clj 6 additions, 0 deletionssrc/metabase/types.clj
- src/metabase/util/ssh.clj 8 additions, 1 deletionsrc/metabase/util/ssh.clj
- test/metabase/db/metadata_queries_test.clj 24 additions, 1 deletiontest/metabase/db/metadata_queries_test.clj
- test/metabase/driver/postgres_test.clj 2 additions, 1 deletiontest/metabase/driver/postgres_test.clj
- test/metabase/driver/sql_jdbc/connection_test.clj 43 additions, 1 deletiontest/metabase/driver/sql_jdbc/connection_test.clj
- test/metabase/sync/analyze/fingerprint/fingerprinters_test.clj 37 additions, 14 deletions...metabase/sync/analyze/fingerprint/fingerprinters_test.clj
- test/metabase/sync/analyze/fingerprint_test.clj 16 additions, 0 deletionstest/metabase/sync/analyze/fingerprint_test.clj
Loading
Please register or sign in to comment