fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBLOB as opposed to BLOB for MySQL #20385

john-bodley · 2022-06-14T23:23:01Z

SUMMARY

At Airbnb we use MySQL for Superset's metadata database where the SQLAlchemy LargeBinary type—which is used for the key_value.value column—is encoded as a BLOB which can handle up to 64 KiB of data. This isn't sufficient for storing large form-data blobs and thus this PR adds a migration which encodes the column as a MEDIUMBLOB which can handle up to 16 MiB of data.

The solution uses the suggestion here, note 16 MiB is equivalent to 2^24 - 1 bytes.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

etr2460 · 2022-06-14T23:26:44Z

superset/migrations/versions/2022-06-14_15-28_e09b4ae78457_.py

+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""empty message


please describe the revision

etr2460 · 2022-06-14T23:27:11Z

superset/migrations/versions/2022-06-14_15-28_e09b4ae78457_.py

@@ -0,0 +1,50 @@
+# Licensed to the Apache Software Foundation (ASF) under one


Please name the migration file descriptively

codecov · 2022-06-14T23:28:57Z

Codecov Report

Merging #20385 (69bdb78) into master (160e674) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #20385   +/-   ##
=======================================
  Coverage   66.68%   66.68%           
=======================================
  Files        1739     1739           
  Lines       65143    65143           
  Branches     6902     6902           
=======================================
  Hits        43443    43443           
  Misses      19947    19947           
  Partials     1753     1753

Flag	Coverage Δ
hive	`53.70% <100.00%> (ø)`
mysql	`82.29% <100.00%> (ø)`
postgres	`82.36% <100.00%> (ø)`
presto	`53.56% <100.00%> (ø)`
python	`82.78% <100.00%> (ø)`
sqlite	`82.09% <100.00%> (ø)`
unit	`50.15% <100.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/key_value/models.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 160e674...69bdb78. Read the comment docs.

…OB as opposed to BLOB for MySQL

etr2460

lgtm, thanks for making this fix in opensource!

ktmud · 2022-06-15T01:02:18Z

I'm pretty sure me and @villebro have discussed about this before... #19805

Unfortunately the fix was only added to the original migration and not to the model. We didn't create a new migration for existing users, either---because the feature was still not in official releases yet.

ktmud · 2022-06-15T01:06:35Z

I'm also wondering whether it's worth creating a new type like MediumText and update key_value/models to point to this type as well. It's risky when the declarative model is out of sync with the actual db schema.

Highlighting this comment since it obviously got lost in the original PR.

etr2460 · 2022-06-15T01:33:41Z

I'm pretty sure me and @villebro have discussed about this before... #19805

Unfortunately the fix was only added to the original migration and not to the model. We didn't create a new migration for existing users, either---because the feature was still not in official releases yet.

🤦 well we ran into it at least. I think it's probably best practice to not modify a merged migration unless there's a bug with it that can't be resolved in a future migration. And even then, maybe it's better to have another migration that undoes the bad one and then performs the same thing with the bug fix

villebro

Well this went totally sideways ☹️ Sorry for the mess. One comment regarding keeping the the length of the migration consistent with revision 6766938c6065. Also, it's good to know for similar future issues that the migration apparently succeeds even if the existing_type isn't accurate (when running migrations from scratch on the current codebase it'll be performing the migration on a column with length=2**31).

villebro · 2022-06-15T06:46:01Z

superset/migrations/versions/2022-06-14_15-28_e09b4ae78457_resize_key_value_blob.py

+            "value",
+            existing_nullable=False,
+            existing_type=sa.LargeBinary(),
+            type_=sa.LargeBinary(length=2**24 - 1),


To keep this in line with #19805, should we make it length=2**31?

@villebro aren't we over provisioning the column if we opt for ~ 2 GiB as opposed to 16 MiB? Maybe per #19805 the existing_type should be sa.LargeBinary(length=2**31)—and something similar in the downgrade.

michael-s-molina · 2022-06-23T11:38:19Z

@john-bodley @villebro Merging this to include it in v2.0. We can add more changes in a follow-up if necessary 😉

…OB as opposed to BLOB for MySQL (#20385) * fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBLOB as opposed to BLOB for MySQL * Update 2022-06-14_15-28_e09b4ae78457_resize_key_value_blob.py Co-authored-by: John Bodley <john.bodley@airbnb.com> (cherry picked from commit f5cb23e)

john-bodley requested a review from a team as a code owner June 14, 2022 23:23

pull-request-size bot added the size/M label Jun 14, 2022

etr2460 reviewed Jun 14, 2022

View reviewed changes

fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBL…

89b890a

…OB as opposed to BLOB for MySQL

john-bodley force-pushed the john-bodley--migration-key_value-blob branch from 2aad4ec to 89b890a Compare June 14, 2022 23:30

etr2460 approved these changes Jun 14, 2022

View reviewed changes

Update 2022-06-14_15-28_e09b4ae78457_resize_key_value_blob.py

69bdb78

villebro reviewed Jun 15, 2022

View reviewed changes

michael-s-molina added the v2.0 label Jun 21, 2022

michael-s-molina merged commit f5cb23e into apache:master Jun 23, 2022

john-bodley referenced this pull request Jul 20, 2022

fix(db): set slices.query_context to MEDIUMTEXT for mysql (#20779)

a64cdec

villebro mentioned this pull request May 9, 2023

fix(migration): handle permalink edge cases correctly #23980

Merged

9 tasks

mistercrunch added 🍒 2.0.0 🍒 2.0.1 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.1.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBLOB as opposed to BLOB for MySQL #20385

fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBLOB as opposed to BLOB for MySQL #20385

john-bodley commented Jun 14, 2022

etr2460 Jun 14, 2022

etr2460 Jun 14, 2022

codecov bot commented Jun 14, 2022 •

edited

Loading

etr2460 left a comment

ktmud commented Jun 15, 2022 •

edited

Loading

ktmud commented Jun 15, 2022

etr2460 commented Jun 15, 2022

villebro left a comment

villebro Jun 15, 2022

john-bodley Jun 21, 2022

michael-s-molina commented Jun 23, 2022

		@@ -0,0 +1,50 @@
		# Licensed to the Apache Software Foundation (ASF) under one

fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBLOB as opposed to BLOB for MySQL #20385

fix(migration): Ensure key_value LargeBinary is encoded as a MEDIUMBLOB as opposed to BLOB for MySQL #20385

Conversation

john-bodley commented Jun 14, 2022

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

etr2460 Jun 14, 2022

Choose a reason for hiding this comment

etr2460 Jun 14, 2022

Choose a reason for hiding this comment

codecov bot commented Jun 14, 2022 • edited Loading

Codecov Report

etr2460 left a comment

Choose a reason for hiding this comment

ktmud commented Jun 15, 2022 • edited Loading

ktmud commented Jun 15, 2022

etr2460 commented Jun 15, 2022

villebro left a comment

Choose a reason for hiding this comment

villebro Jun 15, 2022

Choose a reason for hiding this comment

john-bodley Jun 21, 2022

Choose a reason for hiding this comment

michael-s-molina commented Jun 23, 2022

codecov bot commented Jun 14, 2022 •

edited

Loading

ktmud commented Jun 15, 2022 •

edited

Loading