CSV appends: Use a transaction for inserting data #36995
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
What's your thought on memory usage? |
|
Are you referring to the memory footprint on the JVM, or the database? Memory usage of the JVM should be independent of whether transactions are used or not, because the objects can be garbage collected after they are inserted. As for the database, MySQL and Postgres are smart enough to write large files of data to disk, so memory isn't an issue there either. Otherwise large transactions would be impossible to execute. |
|
I was thinking about database memory usage.
Cool, that's good to know, in that case we don't need to worry about blowing the dataware house up. |
qnkhuat
reviewed
Dec 22, 2023
src/metabase/driver/mysql.clj
Outdated
Comment on lines
751
to
761
| (let [temp-file (File/createTempFile table-name ".tsv") | ||
| file-path (.getAbsolutePath temp-file)] | ||
| (try | ||
| (let [tsvs (map (partial row->tsv driver (count column-names)) values) | ||
| sql (sql/format {::load [file-path (keyword table-name)] | ||
| :columns (map keyword column-names)} | ||
| :quoted true | ||
| :dialect (sql.qp/quote-style driver))] | ||
| (with-open [^java.io.Writer writer (jio/writer file-path)] | ||
| (doseq [value (interpose \newline tsvs)] | ||
| (.write writer (str value)))) |
There was a problem hiding this comment.
this should be done outside of transaction
qnkhuat
reviewed
Dec 22, 2023
src/metabase/driver/sql_jdbc.clj
Outdated
Comment on lines
144
to
160
| (let [table-name (keyword table-name) | ||
| columns (map keyword column-names) | ||
| ;; We need to partition the insert into multiple statements for both performance and correctness. | ||
| ;; | ||
| ;; On Postgres with a large file, 100 (3.76m) was significantly faster than 50 (4.03m) and 25 (4.27m). 1,000 was a | ||
| ;; little faster but not by much (3.63m), and 10,000 threw an error: | ||
| ;; PreparedStatement can have at most 65,535 parameters | ||
| ;; One imagines that `(long (/ 65535 (count columns)))` might be best, but I don't trust the 65K limit to apply | ||
| ;; across all drivers. With that in mind, 100 seems like a safe compromise. | ||
| ;; There's nothing magic about 100, but it felt good in testing. There could well be a better number. | ||
| chunks (partition-all (or driver/*insert-chunk-rows* 100) values) | ||
| sqls (map #(sql/format {:insert-into table-name | ||
| :columns columns | ||
| :values %} | ||
| :quoted true | ||
| :dialect (sql.qp/quote-style driver)) | ||
| chunks)] |
There was a problem hiding this comment.
let's move this preparation outside of the transaction.
qnkhuat
requested changes
Dec 22, 2023
qnkhuat
reviewed
Dec 22, 2023
| (try | ||
| (driver/insert-into! driver (:id database) (table-identifier table) normed-header parsed-rows) | ||
| (catch Throwable e | ||
|
|
1 failed test on run #904 ↗︎
Details:
Review all test suite changes for PR #36995 ↗︎ |
||||||||||||||||||||
calherries
merged commit cdb50f5
into
appends/milestone-0-endpoint
70 of 72 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR will merge into the feature branch for Merge 1 of Milestone 0 (PR)
Epic: Allow appending more data to CSV uploads
product doc
eng doc
This PR mostly preserves observable behaviour. It replaces the logic I created for handling insertion errors to fake transactions with real transactions.
Originally I wanted to use
_mb_row_idfor deleting inserted rows if there was a failure, because I thought the reason we weren't using transactions before was to make insertions faster. Turns out I was wrong, using a transaction is actually slightly faster because a transaction doesn't add much overhead but each commit does.I say "This PR mostly preserves observable behaviour" because it makes one change: if a failure occurs on inserting data and the table didn't have a
_mb_row_idcolumn before, no_mb_row_idcolumn is created. That behaviour can be seen inappend-no-mb-row-id-failure-test.