Faster sync-fields #38828

calherries · 2024-02-15T19:15:59Z

This is part 2 towards resolving #38492
Part 1 is #38970

This PR adds an alternative implementation of the sync-fields step of sync where we sync the foreign key information of a database.

It adds a new driver feature: describe-fields. If a driver supports this feature, they need to implement the new driver method metabase.driver/describe-fields. If the driver doesn't support the feature, we'll continue to use metabase.driver/describe-table. The plan for driver authors to implement describe-fields in terms of driver/describe-table, which should be a really simple change for most drivers, assuming it doesn't need to be performant for large DBs.

The way this works is instead of querying the customer DB one table at a time using the getColumns JDBC method, we execute a single query that gets all the data at once with paginated results. We then reduce over those results, updating our App DB one foreign key at a time. This approach mirrors #38970

I've tested this with the "massive" redshift instance, and this is how long each sync step took:

"sync-tables": 13.3 s
"sync-fields": 1.0 mins (this is the step we're speeding up)
"sync-fks": 1.3 s (there are no FKs to sync though)
total time for all sync metadata steps: 5.0 minutes

Before this PR, sync-fields took hours (too long to bother measuring precisely anyway).

replay-io · 2024-02-15T19:27:41Z

Status	Complete ↗︎
Commit	`ea9f93e`
Results	⚠️ 2 Flaky ✅ 2359 Passed

qnkhuat · 2024-03-14T08:43:24Z

modules/drivers/redshift/src/metabase/driver/redshift.clj

+                        [nil :database-is-auto-increment] ; only needed for actions, which redshift doesn't support yet
+                        [nil :database-required]          ; only needed for actions, which redshift doesn't support yet


probably don't even need to return it as these fields are optional

Good point, 14fb6a5

calherries · 2024-03-14T08:52:26Z

src/metabase/util/malli/schema.clj

+(defn CollectionOf
+  "Helper for creating schemas to check whether something is an instance of a collection."
+  [item-schema]
+  [:fn
+   {:error/message (format "Collection of %s" item-schema)}
+   #(and (coll? %) (every? (partial mc/validate item-schema) %))])


This is actually dead code, but I think it's generally useful so I'm inclined to keep it around.

At the moment we have no way to type something that could be a set or a vector for example.

Surprised it's not a built-in!

crisptrutski

Getting notified that diffs are out of date, flushing comments before I continue.

crisptrutski · 2024-03-14T08:22:50Z

modules/drivers/redshift/src/metabase/driver/redshift.clj

@@ -87,9 +121,15 @@
 ;; custom Redshift type handling

 (def ^:private database-type->base-type
-  (sql-jdbc.sync/pattern-based-database-type->base-type
-   [[#"(?i)CHARACTER VARYING" :type/Text]       ; Redshift uses CHARACTER VARYING (N) as a synonym for VARCHAR(N)
-    [#"(?i)NUMERIC"           :type/Decimal]])) ; and also has a NUMERIC(P,S) type, which is the same as DECIMAL(P,S)


nit: Why not keep this comment on NUMERIC(P, S)?

Thanks, this got deleted by accident 9edacd0

crisptrutski · 2024-03-14T08:25:07Z

modules/drivers/redshift/src/metabase/driver/redshift.clj

+  (some-fn (sql-jdbc.sync/pattern-based-database-type->base-type
+            [[#"(?i)CHARACTER VARYING" :type/Text]       ; Redshift uses CHARACTER VARYING (N) as a synonym for VARCHAR(N)
+             [#"(?i)NUMERIC"           :type/Decimal]])
+           {:super       :type/*    ; (requested support in metabase#36642)


It would be nice to have a short comment here explaining how these "*" types are treated by the application, and that we only list these mappings here to disable warnings in the logs.

I fear that type of comment could get out of date here. If you have any questions about type/* it should be documented metabase.types namespace. I see that it isn't, but I don't think I could do it justice if I tried right now

I'm mostly listing these types to document these types are deliberately not being handled, as opposed to accidentally not being handled. Removing the log warnings is a nice side-effect though.

crisptrutski · 2024-03-14T08:27:36Z

modules/drivers/redshift/test/metabase/driver/redshift_test.clj

@@ -241,8 +241,8 @@
               view-nm))
          (let [table-id (t2/select-one-pk Table :db_id (u/the-id database), :name view-nm)]
              ;; and its columns' :base_type should have been identified correctly
-            (is (= [{:name "numeric_col",   :database_type "numeric(10,2)",         :base_type :type/Decimal}
-                    {:name "weird_varchar", :database_type "character varying(50)", :base_type :type/Text}]
+            (is (= [{:name "numeric_col",   :database_type "numeric",         :base_type :type/Decimal}


Question: are there any consequences to us no longer recording the size of the types?

nit: I've been fooled by bad GH rendering before, but it looks like :base_type is unaligned now.

As far as I'm aware, we don't use the size / precision data from these strings anywhere.

src/metabase/driver.clj

src/metabase/sync/fetch_metadata.clj

src/metabase/sync/sync_metadata/fields.clj

qnkhuat · 2024-03-14T09:06:08Z

.clj-kondo/config.edn

@@ -355,7 +355,7 @@
    metabase.sync.field-values                                      field-values
    metabase.sync.interface                                         i
    metabase.sync.schedules                                         sync.schedules
-    metabase.sync.sync-metadata.fields.fetch-metadata               fetch-metadata
+    metabase.sync.sync-metadata.fields.our-metadata                 our-metadata


Suggested change

metabase.sync.sync-metadata.fields.our-metadata our-metadata

metabase.sync.sync-metadata.fields.our-metadata fields.our-metadata

without this prefix, it's hard to know whether it's fields metadata or table metadata or db metadata

agreed. we don't use this namespace much so the extra characters doesn't have much downside
40df070

qnkhuat · 2024-03-14T09:10:04Z

src/metabase/sync/sync_metadata/fields.clj

+
+(mu/defn sync-fields-for-db!
+  "Sync the Fields in the Metabase application database for a specific `table`."
+  [database :- i/DatabaseInstance]


you can make this take tables and create a [schema name] -> table mapping that you can use inside the transducer to get the table.

Always good to reduce the number of DB calls

I was also bothered by the N+1 table queries, but ended up not commenting as for large N we'd rather not fetch all the tables into memory.

It seems unfortunate to me that there's a deep nest of methods to require a full table model, because in practice they're mostly using the id, name, and schema mostly. I suspect we could just add one or two more table fields to the educed rows and get rid of this query, but it would be brittle to use a partially populated instance, and refactoring all those methods to depend only on their required table fields would be pretty hairy and ugly.

What Chris said, I'm fetching each table because I'm avoiding fetching all the tables into memory. The good thing is app DB queries should be quite cheap compared to customer DB queries, so this is still a huge win.

In the future we can avoid the N+1 by doing a refactoring like the one I did for sync-fks as I did here https://github.com/metabase/metabase/pull/39679/files#diff-e09c063342055336a262f66623a6a1783a89871e4426c33c92f8f9b5f2bcaaec

But I don't think it's worth it right now. That refactoring took longer than I thought because of the quirks between different app DBs.

okay, that makes sense; in that case, you need to "not" fetch all tables in metabase.sync.sync-metadata.fields line 117

Ah, I see why you made that comment now. Nice spot. Yes that was left over from a previous version where I was doing what you described.

crisptrutski

Nicely done 👌

crisptrutski · 2024-03-14T09:03:14Z

src/metabase/util/malli/schema.clj

+(defn CollectionOf
+  "Helper for creating schemas to check whether something is an instance of a collection."
+  [item-schema]
+  [:fn
+   {:error/message (format "Collection of %s" item-schema)}
+   #(and (coll? %) (every? (partial mc/validate item-schema) %))])


Surprised it's not a built-in!

test/metabase/driver/sql_jdbc/sync/describe_table_test.clj

src/metabase/sync/sync_metadata/fields.clj

qnkhuat · 2024-03-14T09:18:34Z

docs/developers-guide/driver-changelog.md

@@ -4,6 +4,16 @@ title: Driver interface changelog

 # Driver Interface Changelog

+## Metabase 0.49.1


Oh, didn't you tell me that we don't introduce driver methods in the patch releases?

Yeah, maybe I did say that :) I can't remember how I justified it though, probably because we've never done it before. But putting it in the 0.49.0 section doesn't make much sense.

calherries · 2024-03-14T09:53:06Z

@qnkhuat can you take another look? It would be nice to get this in 49.0. It could be if it were approved in the next few hours I think. If it does, I'll update the changelog in the backport PR.

qnkhuat

Resolve this then it LGTM!

calherries · 2024-03-14T10:17:27Z

@qnkhuat done, I also squeezed this one in: ea9f93e. In theory PKs should be optional because they'll default to false for databases that don't support them.

github-actions · 2024-03-14T11:54:42Z

@calherries Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone?

Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>

calherries requested a review from camsaul as a code owner February 15, 2024 19:16

metabase-bot bot assigned calherries Feb 15, 2024

metabase-bot bot added the .Team/BackendComponents also known as BEC label Feb 15, 2024

calherries requested a review from metamben February 15, 2024 19:16

calherries added the no-backport Do not backport this PR to any branch label Feb 16, 2024

metabase deleted a comment from luizarakaki Feb 16, 2024

calherries force-pushed the fast-sync-fields branch from 9ee866a to b444854 Compare February 20, 2024 14:56

calherries mentioned this pull request Feb 20, 2024

Faster sync-fks #38970

Merged

calherries mentioned this pull request Mar 1, 2024

Getting fields in (Redshift) is slow #38492

Closed

calherries added 20 commits March 6, 2024 11:52

Make mark-fk! faster and update existing FKs

4d5eefa

Add foreign key update test

f5af96d

Tidy

650f3bc

Comment out failing test

0265654

add issue #

e2cd368

Make sync-fks! reducible

4b8610e

remove call count

80d01a5

tidy

da4d3a4

Deduplicate

d479731

set semantic_type

04d71be

Use jdbc/execute! not toucan

23f11bd

Fix test

40f1ecd

Fix

e863a2d

Fix

a7488e1

Faster sync-fks

e8075d0

Set initial table sync complete in sync fks

46c1672

Fix docstring of get-fks

e531d05

Tidy up and add schema for fk-metadata

c1589ac

Add back describe-table-fks implementation

dc14c44

Switch to redshift and order by FK schema and table

370249b

Change driver changelog entry to 49.1

137c140

qnkhuat reviewed Mar 14, 2024

View reviewed changes

calherries commented Mar 14, 2024

View reviewed changes

Merge branch 'master' into fast-sync-fields

70dd1a3

crisptrutski reviewed Mar 14, 2024

View reviewed changes

calherries added 2 commits March 14, 2024 11:01

Add explanation for missing columns

14fb6a5

Add back missing comment

9edacd0

qnkhuat reviewed Mar 14, 2024

View reviewed changes

crisptrutski approved these changes Mar 14, 2024

View reviewed changes

qnkhuat reviewed Mar 14, 2024

View reviewed changes

calherries added 5 commits March 14, 2024 11:23

alignment

daf086d

reference to multimethods in features

0b1742f

Use mu.fn/instrument-ns?

e10924a

alignment

4cdc40e

fields.our-metadata

40df070

calherries requested a review from qnkhuat March 14, 2024 09:51

qnkhuat approved these changes Mar 14, 2024

View reviewed changes

calherries added 2 commits March 14, 2024 12:12

Don't fetch all tables

f0bedd4

pk? is only required for DBs with PKs, so leave it optional

ea9f93e

calherries enabled auto-merge (squash) March 14, 2024 10:17

calherries added backport Automatically create PR on current release branch on merge and removed no-backport Do not backport this PR to any branch labels Mar 14, 2024

calherries merged commit 37b5067 into master Mar 14, 2024
120 of 133 checks passed

calherries deleted the fast-sync-fields branch March 14, 2024 11:54

github-actions bot pushed a commit that referenced this pull request Mar 14, 2024

Faster sync-fields (#38828)

c77690f

metabase-bot bot mentioned this pull request Mar 14, 2024

🤖 backported "Faster sync-fields" #40123

Merged

metabase-bot bot added a commit that referenced this pull request Mar 14, 2024

Faster sync-fields (#38828) (#40123)

f537061

Co-authored-by: Cal Herries <39073188+calherries@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Faster sync-fields #38828

Faster sync-fields #38828

calherries commented Feb 15, 2024 •

edited

Loading

replay-io bot commented Feb 15, 2024 •

edited

Loading

qnkhuat Mar 14, 2024

calherries Mar 14, 2024

calherries Mar 14, 2024

crisptrutski Mar 14, 2024

crisptrutski left a comment

crisptrutski Mar 14, 2024

calherries Mar 14, 2024

crisptrutski Mar 14, 2024

calherries Mar 14, 2024

crisptrutski Mar 14, 2024

calherries Mar 14, 2024

qnkhuat Mar 14, 2024

calherries Mar 14, 2024

qnkhuat Mar 14, 2024 •

edited

Loading

crisptrutski Mar 14, 2024

calherries Mar 14, 2024 •

edited

Loading

qnkhuat Mar 14, 2024

calherries Mar 14, 2024

crisptrutski left a comment

crisptrutski Mar 14, 2024

qnkhuat Mar 14, 2024

calherries Mar 14, 2024

calherries commented Mar 14, 2024 •

edited

Loading

qnkhuat left a comment

calherries commented Mar 14, 2024 •

edited

Loading

github-actions bot commented Mar 14, 2024

		[nil :database-is-auto-increment] ; only needed for actions, which redshift doesn't support yet
		[nil :database-required] ; only needed for actions, which redshift doesn't support yet

	metabase.sync.sync-metadata.fields.our-metadata our-metadata
	metabase.sync.sync-metadata.fields.our-metadata fields.our-metadata

		@@ -4,6 +4,16 @@ title: Driver interface changelog

		# Driver Interface Changelog

		## Metabase 0.49.1

Faster sync-fields #38828

Faster sync-fields #38828

Conversation

calherries commented Feb 15, 2024 • edited Loading

replay-io bot commented Feb 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

qnkhuat Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calherries Mar 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crisptrutski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calherries commented Mar 14, 2024 • edited Loading

qnkhuat left a comment

Choose a reason for hiding this comment

calherries commented Mar 14, 2024 • edited Loading

github-actions bot commented Mar 14, 2024

calherries commented Feb 15, 2024 •

edited

Loading

replay-io bot commented Feb 15, 2024 •

edited

Loading

qnkhuat Mar 14, 2024 •

edited

Loading

calherries Mar 14, 2024 •

edited

Loading

calherries commented Mar 14, 2024 •

edited

Loading

calherries commented Mar 14, 2024 •

edited

Loading