Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Optimise autocomplete query #38080

Merged
merged 10 commits into from
Jan 26, 2024
Merged

Optimise autocomplete query #38080

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
65daf4c
Filter for active tables only
romeovs Jan 19, 2024
dc83b6b
Switch to more efficient inner-join
romeovs Jan 19, 2024
ac0e2b5
move check for active table inside the join
piranha Jan 24, 2024
732c888
add index for autocomplete search on fields
piranha Jan 24, 2024
6e28535
h2 can't do indexes on functions
piranha Jan 24, 2024
13c31ce
ok, checking for active fields breaks tests
piranha Jan 24, 2024
fde4b11
no indexes on expression for me, at least I can set Cache-Control
piranha Jan 24, 2024
fdbfe60
explain caching setup
piranha Jan 25, 2024
ddcba4e
add back index at least for postgres
piranha Jan 25, 2024
6512c94
add comment about index in pgsql
piranha Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 15 additions & 9 deletions src/metabase/api/database.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,11 @@
:%lower.metabase_field/name [:like (u/lower-case-en search-string)]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth commenting that an index on lower(name) dramatically speeds the query up in DBs that support it, but we left it for now since Maria and H2 do not support indexes on computed fields. Perhaps they'll catch up in future? I also wonder if there are other use cases for a lowercase username, we could always materialize this as another column.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's actually not a filter problem, but an ordering one (weirdly). On stats adding a ngram index sped up query for 80 to 20 ms, but adding a index (which is used for sorting) speeds it up to 10 ms (no difference if with or without ngram index). 🤯

:metabase_field.visibility_type [:not-in ["sensitive" "retired"]]
:table.db_id db-id
{:order-by [[[:lower :metabase_field.name] :asc]
[[:lower :table.name] :asc]]
:left-join [[:metabase_table :table] [:= :table.id :metabase_field.table_id]]
:limit limit}))
{:order-by [[[:lower :metabase_field.name] :asc]
[[:lower :table.name] :asc]]
:inner-join [[:metabase_table :table] [:and :table.active
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be worth commenting that it was (counter-intuitively) faster to have the "active" check within the join, in case someone comes to optimize again.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hah, it felt intuitive for me - you get less data to join this way 😁 But sure, I can leave a comment there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably you're right, my intuition is quite rusty and I didn't give much thought to it 😆

[:= :table.id :metabase_field.table_id]]]
:limit limit}))

(defn- autocomplete-results [tables fields limit]
(let [tbl-count (count tables)
Expand Down Expand Up @@ -620,11 +621,16 @@
(when (and (str/blank? prefix) (str/blank? substring))
(throw (ex-info (tru "Must include prefix or search") {:status-code 400})))
(try
(cond
substring
(autocomplete-suggestions id (str "%" substring "%"))
prefix
(autocomplete-suggestions id (str prefix "%")))
{:status 200
;; Presumably user will repeat same prefixes many times writing the query,
;; so let them cache response to make autocomplete feel fast. 60 seconds
;; is not enough to be a nuisance when schema or permissions change. Cache
;; is user-specific since we're checking for permissions.
:headers {"Cache-Control" "public, max-age=60"
"Vary" "Cookie"}
:body (cond
substring (autocomplete-suggestions id (str "%" substring "%"))
prefix (autocomplete-suggestions id (str prefix "%")))}
Comment on lines +629 to +638
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm a sucker for small methods and avoiding mixing too many levels of abstraction. I'd break out a small with-user-specific-cache-header method which takes the body.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK, I'd rather this be "in the face" until a general pattern emerges...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rule of 3? Fine 😄

(catch Throwable e
(log/warn e (trs "Error with autocomplete: {0}" (ex-message e))))))

Expand Down
9 changes: 5 additions & 4 deletions src/metabase/server/middleware/security.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,11 @@
"X-Content-Type-Options" "nosniff"}))

(defn- add-security-headers* [request response]
(update response :headers merge (security-headers
:nonce (:nonce request)
:allow-iframes? ((some-fn request.u/public? request.u/embed?) request)
:allow-cache? (request.u/cacheable? request))))
;; merge is other way around so that handler can override headers
(update response :headers #(merge %2 %1) (security-headers
:nonce (:nonce request)
:allow-iframes? ((some-fn request.u/public? request.u/embed?) request)
:allow-cache? (request.u/cacheable? request))))

(defn add-security-headers
"Middleware that adds HTTP security and cache-busting headers."
Expand Down
9 changes: 9 additions & 0 deletions test/metabase/api/database_test.clj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
[metabase.driver.h2 :as h2]
[metabase.driver.sql-jdbc.execute :as sql-jdbc.execute]
[metabase.driver.util :as driver.u]
[metabase.http-client :as client]
[metabase.lib.schema.id :as lib.schema.id]
[metabase.models
:refer [Card Collection Database Field FieldValues Metric Segment Table]]
Expand All @@ -27,6 +28,7 @@
[metabase.test :as mt]
[metabase.test.data.impl :as data.impl]
[metabase.test.data.interface :as tx]
[metabase.test.data.users :as test.users]
[metabase.test.fixtures :as fixtures]
[metabase.test.util :as tu]
[metabase.util :as u]
Expand Down Expand Up @@ -683,6 +685,13 @@
["CATEGORY" "PRODUCTS :type/Text :type/Category"]
["CATEGORY_ID" "VENUES :type/Integer :type/FK"]]}]
(is (= expected (prefix-fn (mt/id) prefix))))
(testing " returns sane Cache-Control headers"
(is (=? {"Cache-Control" "public, max-age=30"
piranha marked this conversation as resolved.
Show resolved Hide resolved
"Vary" "Cookie"}
(-> (client/client-full-response (test.users/username->token :rasta) :get 200
(format "database/%s/autocomplete_suggestions" (mt/id))
:prefix "u")
:headers))))
(testing " handles large numbers of tables and fields sensibly with prefix"
(mt/with-model-cleanup [Field Table Database]
(let [tmp-db (first (t2/insert-returning-instances! Database {:name "Temp Autocomplete Pagination DB" :engine "h2" :details "{}"}))]
Expand Down