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.

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

🍒 Fix remaining tinyint booleans in MySQL (Backport #43296 to v49) #43781

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

johnswanson
Copy link
Contributor

Liquibase changed their boolean type in MySQL from bit(1) to
tinyint(4) in version 4.25.1. Our JDBC driver does not recognize these
as booleans, so we needed to migrate them to bit(1)s.

As discussed here, we
changed all existing boolean types that were in the
001_update_migrations.yml but not the SQL initialization file.

For new installations, this works: things in the SQL initialization file
get created with the bit(1) type.

However, for existing installations, there's a potential issue. Say I'm
on v42 and am upgrading to v49. In v43, a new boolean was added.

In this case, I'll get the boolean from the liquibase migration rather
than from the SQL initialization file, and it need to be changed to a
bit(1).

I installed Metabase v41 with MySQL, migrated the database, and then
installed Metabase v49 and migrated again. I made a list of all the
columns that had the type tinyint:

mysql> SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, COLUMN_TYPE,        COLUMN_DEFAULT, IS_NULLABLE FROM INFORMATION_SCHEMA.COLUMNS WHERE COLUMN_TYPE = 'tinyint' AND TABLE_SCHEMA='metabase_test';
+---------------+------------------------------+-------------------+-------------+----------------+-------------+
| TABLE_SCHEMA  | TABLE_NAME                   | COLUMN_NAME       | COLUMN_TYPE | COLUMN_DEFAULT | IS_NULLABLE |
+---------------+------------------------------+-------------------+-------------+----------------+-------------+
| metabase_test | core_user                    | is_datasetnewb    | tinyint     | 1              | NO          |
| metabase_test | metabase_field               | database_required | tinyint     | 0              | NO          |
| metabase_test | metabase_fieldvalues         | has_more_values   | tinyint     | 0              | YES         |
| metabase_test | permissions_group_membership | is_group_manager  | tinyint     | 0              | NO          |
| metabase_test | persisted_info               | active            | tinyint     | 0              | NO          |
| metabase_test | report_card                  | dataset           | tinyint     | 0              | NO          |
| metabase_test | timeline                     | archived          | tinyint     | 0              | NO          |
| metabase_test | timeline                     | default           | tinyint     | 0              | NO          |
| metabase_test | timeline_event               | archived          | tinyint     | 0              | NO          |
| metabase_test | timeline_event               | time_matters      | tinyint     | NULL           | NO          |
+---------------+------------------------------+-------------------+-------------+----------------+-------------+
10 rows in set (0.01 sec)

Then wrote migrations. For each column, we:

  • turn it into a bit(1),

  • re-set the previously existing default value, and

  • re-add the NOT NULL constraint, if applicable.

@metabase-bot metabase-bot bot added the .Team/AdminWebapp Admin and Webapp team label Jun 6, 2024
* Correct type for `report_card.dataset`

This was missed in the original migration of boolean types to
${boolean.type} in MySQL/MariaDB, and then missed again by me when I
migrated `collection_preview` over a week ago.

* Change all boolean types to `bit(1)` in MySQL

Liquibase changed their boolean type in MySQL from `bit(1)` to
`tinyint(4)` in version 4.25.1. Our JDBC driver does not recognize these
as booleans, so we needed to migrate them to `bit(1)`s.

As discussed [here](#36964), we
changed all existing `boolean` types that were in the
`001_update_migrations.yml` but not the SQL initialization file.

For new installations, this works: things in the SQL initialization file
get created with the `bit(1)` type.

However, for existing installations, there's a potential issue. Say I'm
on v42 and am upgrading to v49. In v43, a new `boolean` was added.

In this case, I'll get the `boolean` from the liquibase migration rather
than from the SQL initialization file, and it need to be changed to a
`bit(1)`.

I installed Metabase v41 with MySQL, migrated the database, and then
installed Metabase v49 and migrated again. I made a list of all the
columns that had the type `tinyint`:

```
mysql> SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, COLUMN_TYPE,        COLUMN_DEFAULT, IS_NULLABLE FROM INFORMATION_SCHEMA.COLUMNS WHERE COLUMN_TYPE = 'tinyint' AND TABLE_SCHEMA='metabase_test';
+---------------+------------------------------+-------------------+-------------+----------------+-------------+
| TABLE_SCHEMA  | TABLE_NAME                   | COLUMN_NAME       | COLUMN_TYPE | COLUMN_DEFAULT | IS_NULLABLE |
+---------------+------------------------------+-------------------+-------------+----------------+-------------+
| metabase_test | core_user                    | is_datasetnewb    | tinyint     | 1              | NO          |
| metabase_test | metabase_field               | database_required | tinyint     | 0              | NO          |
| metabase_test | metabase_fieldvalues         | has_more_values   | tinyint     | 0              | YES         |
| metabase_test | permissions_group_membership | is_group_manager  | tinyint     | 0              | NO          |
| metabase_test | persisted_info               | active            | tinyint     | 0              | NO          |
| metabase_test | report_card                  | dataset           | tinyint     | 0              | NO          |
| metabase_test | timeline                     | archived          | tinyint     | 0              | NO          |
| metabase_test | timeline                     | default           | tinyint     | 0              | NO          |
| metabase_test | timeline_event               | archived          | tinyint     | 0              | NO          |
| metabase_test | timeline_event               | time_matters      | tinyint     | NULL           | NO          |
+---------------+------------------------------+-------------------+-------------+----------------+-------------+
10 rows in set (0.01 sec)
```

Then wrote migrations. For each column, we:

- turn it into a `bit(1)`,

- re-set the previously existing default value, and

- re-add the NOT NULL constraint, if applicable.

* Change author and add missing `dbms`

---------

Co-authored-by: John Swanson <john.swanson@metabase.com>
@nemanjaglumac nemanjaglumac changed the title Fix remaining tinyint booleans in MySQL (#43296) (Backport #43296) 🍒 Fix remaining tinyint booleans in MySQL (#43296) (Backport #43296 to v49) Jun 6, 2024
@nemanjaglumac nemanjaglumac changed the title 🍒 Fix remaining tinyint booleans in MySQL (#43296) (Backport #43296 to v49) 🍒 Fix remaining tinyint booleans in MySQL (Backport #43296 to v49) Jun 6, 2024
@nemanjaglumac nemanjaglumac added the was-backported apply this to PRs that are themselves backports label Jun 6, 2024
Copy link

replay-io bot commented Jun 6, 2024

Status Complete ↗︎
Commit 8ee4042
Results
⚠️ 2 Flaky
2374 Passed

@johnswanson johnswanson merged commit 5eb2650 into release-x.49.x Jun 7, 2024
111 checks passed
@johnswanson johnswanson deleted the backport-43296-v49 branch June 7, 2024 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
.Team/AdminWebapp Admin and Webapp team was-backported apply this to PRs that are themselves backports
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants