Page MenuHomePhabricator

Enable MariaDB/MySQL's Strict Mode
Open, MediumPublic

Description

If you are looking for the full RFC, go to T112637

This task's scope is:

  • switching to MariaDB's Strict Mode, first on CI (T119371), then on production
  • Make it so that DB insert/update that would overflow would error out rather than be silently truncated
  • Generally promote better use of MariaDB by making many things fail precommit and/or fail loudly in production rather than failing quietly

Options discussed:

  • sql_mode = 'TRADITIONAL' (equivalent to STRICT_TRANS_TABLES, STRICT_ALL_TABLES, NO_ZERO_IN_DATE, NO_ZERO_DATE, ERROR_FOR_DIVISION_BY_ZERO, NO_AUTO_CREATE_USER)
  • sql_mode = ONLY_FULL_GROUP_BY

Often, our non-strict usage is a fountain of bugs and security issues.

Slide by @jcrespo that explains it: https://www.slideshare.net/jynus/query-optimization-with-mysql-57-and-mariadb-10-even-newer-tricks/43 and https://www.slideshare.net/jynus/query-optimization-with-mysql-57-and-mariadb-10-even-newer-tricks/44

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
OpenNone
Resolvedhashar
ResolvedKrinkle
OpenNone
ResolvedNone
ResolvedNikerabbit
Resolved RobLa-WMF
OpenNone
ResolvedNikerabbit
ResolvedEjegg
OpenNone
ResolvedMarostegui
OpenNone
ResolvedTK-999
ResolvedNone
OpenBUG REPORTNone
OpenBUG REPORTNone
OpenNone
In ProgressBUG REPORTDiesel_kapasule

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
hashar triaged this task as Medium priority.Nov 2 2015, 8:35 PM

Poked Jaime about it by email.

Strict mode is really useful, because it helped me to find a data loss issue T117854: Missing updater for rc_ip field: RecentChange::save: 1406 Data too long for column 'rc_ip' (the fact that it has not even been triaged yet annoys me).

But on the other hand, currently trying to do development with strict mode enabled is difficult. Just today when testing a patch I ran into T121489: Field 'ar_page' doesn't have a default value and T67807: Incorrect integer value: '' for column 'af_hidden'.

currently trying to do development with strict mode enabled is difficult

@Nikerabbit I assume you are working locally enabling strict mode, right? As far as I can tell, strict sql mode has not been enabled neither in production, not in beta, nor in CI tests yet. However, those are problems that other instances of mediawiki outside of the WMF could easily find, and we should prioritize fixing them. I think changing beta so that it is stricter than production would be a good way to debug them.

Correct. I am using strict mode locally to make sure we don't introduce any new strict mode errors and catch any such errors in extensions we maintain.

This is currently unassigned. Is that on purpose? I'm not sure if @hashar, @jcrespo , or someone else is the most logical assignee.

One potential issue I see with ONLY_FULL_GROUP_BY is that the optimizer still sucks at dealing with it:

Without ONLY_FULL_GROUP_BY
mysql:wikiadmin@db1083 [enwiki]> explain SELECT page_namespace, page_title, page_id, page_is_redirect FROM page WHERE page_namespace=0 AND page_title>='@' GROUP BY page_title LIMIT 11;
+------+-------------+-------+------+---------------+------------+---------+-------+----------+------------------------------------+
| id   | select_type | table | type | possible_keys | key        | key_len | ref   | rows     | Extra                              |
+------+-------------+-------+------+---------------+------------+---------+-------+----------+------------------------------------+
|    1 | SIMPLE      | page  | ref  | name_title    | name_title | 4       | const | 20324890 | Using index condition; Using where |
+------+-------------+-------+------+---------------+------------+---------+-------+----------+------------------------------------+
1 row in set (0.00 sec)

mysql:wikiadmin@db1083 [enwiki]> SELECT page_namespace, page_title, page_id, page_is_redirect FROM page WHERE page_namespace=0 AND page_title>='@' GROUP BY page_title LIMIT 11;
+----------------+----------------------+----------+------------------+
| page_namespace | page_title           | page_id  | page_is_redirect |
+----------------+----------------------+----------+------------------+
|              0 | @                    |  2214518 |                1 |
|              0 | @!                   | 37696354 |                1 |
|              0 | @$$                  | 21089863 |                1 |
|              0 | @&*!_Smilers         | 20573031 |                1 |
|              0 | @-Bristol            | 23905089 |                1 |
|              0 | @-symbol             | 25176688 |                1 |
|              0 | @440                 | 18159966 |                1 |
|              0 | @ANCAlerts           | 47685646 |                0 |
|              0 | @Andheri             | 42866631 |                0 |
|              0 | @Andheri_(2014_film) | 42914733 |                1 |
|              0 | @BarackObama         | 35944823 |                1 |
+----------------+----------------------+----------+------------------+
11 rows in set (0.09 sec)

mysql:wikiadmin@db1083 [enwiki]> SHOW STATUS LIKE 'Hand%';
+----------------------------+--------+
| Variable_name              | Value  |
+----------------------------+--------+
| Handler_commit             | 2      |
| Handler_delete             | 0      |
| Handler_discover           | 0      |
| Handler_external_lock      | 0      |
| Handler_icp_attempts       | 497545 |
| Handler_icp_match          | 16     |
| Handler_mrr_init           | 0      |
| Handler_mrr_key_refills    | 0      |
| Handler_mrr_rowid_refills  | 0      |
| Handler_prepare            | 0      |
| Handler_read_first         | 0      |
| Handler_read_key           | 1      |
| Handler_read_last          | 0      |
| Handler_read_next          | 11     |
| Handler_read_prev          | 0      |
| Handler_read_rnd           | 0      |
| Handler_read_rnd_deleted   | 0      |
| Handler_read_rnd_next      | 168    |
| Handler_rollback           | 0      |
| Handler_savepoint          | 0      |
| Handler_savepoint_rollback | 0      |
| Handler_tmp_update         | 0      |
| Handler_tmp_write          | 166    |
| Handler_update             | 0      |
| Handler_write              | 0      |
+----------------------------+--------+
25 rows in set (0.00 sec)
With ONLY_FULL_GROUP_BY
mysql:wikiadmin@db1083 [enwiki]> SET SESSION sql_mode = 'TRADITIONAL,ONLY_FULL_GROUP_BY';
Query OK, 0 rows affected (0.00 sec)

mysql:wikiadmin@db1083 [enwiki]> explain SELECT page_namespace, page_title, page_id, page_is_redirect FROM page WHERE page_namespace=0 AND page_title>='@' GROUP BY page_title LIMIT 11;
ERROR 1055 (42000): 'enwiki.page.page_namespace' isn't in GROUP BY
mysql:wikiadmin@db1083 [enwiki]> explain SELECT page_namespace, page_title, page_id, page_is_redirect FROM page WHERE page_namespace=0 AND page_title>='@' GROUP BY page_namespace, page_title, page_id, page_is_redirect LIMIT 11;
+------+-------------+-------+------+---------------+------------+---------+-------+----------+----------------------------------------------------+
| id   | select_type | table | type | possible_keys | key        | key_len | ref   | rows     | Extra                                              |
+------+-------------+-------+------+---------------+------------+---------+-------+----------+----------------------------------------------------+
|    1 | SIMPLE      | page  | ref  | name_title    | name_title | 4       | const | 20324891 | Using index condition; Using where; Using filesort |
+------+-------------+-------+------+---------------+------------+---------+-------+----------+----------------------------------------------------+
1 row in set (0.00 sec)

mysql:wikiadmin@db1083 [enwiki]> SELECT page_namespace, page_title, page_id, page_is_redirect FROM page WHERE page_namespace=0 AND page_title>='@' GROUP BY page_namespace, page_title, page_id, page_is_redirect LIMIT 11;
+----------------+----------------------+----------+------------------+
| page_namespace | page_title           | page_id  | page_is_redirect |
+----------------+----------------------+----------+------------------+
|              0 | @                    |  2214518 |                1 |
|              0 | @!                   | 37696354 |                1 |
|              0 | @$$                  | 21089863 |                1 |
|              0 | @&*!_Smilers         | 20573031 |                1 |
|              0 | @-Bristol            | 23905089 |                1 |
|              0 | @-symbol             | 25176688 |                1 |
|              0 | @440                 | 18159966 |                1 |
|              0 | @ANCAlerts           | 47685646 |                0 |
|              0 | @Andheri             | 42866631 |                0 |
|              0 | @Andheri_(2014_film) | 42914733 |                1 |
|              0 | @BarackObama         | 35944823 |                1 |
+----------------+----------------------+----------+------------------+
11 rows in set (32.67 sec)

mysql:wikiadmin@db1083 [enwiki]> SHOW STATUS LIKE 'Hand%';
+----------------------------+----------+
| Variable_name              | Value    |
+----------------------------+----------+
| Handler_commit             | 2        |
| Handler_delete             | 0        |
| Handler_discover           | 0        |
| Handler_external_lock      | 0        |
| Handler_icp_attempts       | 12637141 |
| Handler_icp_match          | 12139612 |
| Handler_mrr_init           | 0        |
| Handler_mrr_key_refills    | 0        |
| Handler_mrr_rowid_refills  | 0        |
| Handler_prepare            | 0        |
| Handler_read_first         | 0        |
| Handler_read_key           | 1        |
| Handler_read_last          | 0        |
| Handler_read_next          | 12139611 |
| Handler_read_prev          | 0        |
| Handler_read_rnd           | 0        |
| Handler_read_rnd_deleted   | 0        |
| Handler_read_rnd_next      | 168      |
| Handler_rollback           | 1        |
| Handler_savepoint          | 0        |
| Handler_savepoint_rollback | 0        |
| Handler_tmp_update         | 0        |
| Handler_tmp_write          | 166      |
| Handler_update             | 0        |
| Handler_write              | 0        |
+----------------------------+----------+
25 rows in set (0.00 sec)

That was a silly toy example and it still took a lot longer to return the 11 rows. The optimizer seems to fail in the same way if the GROUP BY contains any fields that aren't part of the index being used for grouping (or that are but are constant in the WHERE clause).

The particularly fun part about this example is that "WHERE page_namespace=0 GROUP BY page_title" already uniquely specifies a result row since name_title is a unique index.

I suppose we could always cheat...

mysql:wikiadmin@db1083 [enwiki]> SELECT 0 AS page_namespace, page_title, MAX(page_id) AS page_id, MAX(page_is_redirect) AS page_is_redirect FROM page WHERE page_namespace=0 AND page_title>='@' GROUP BY page_title LIMIT 11;
+----------------+----------------------+----------+------------------+
| page_namespace | page_title           | page_id  | page_is_redirect |
+----------------+----------------------+----------+------------------+
|              0 | @                    |  2214518 |                1 |
|              0 | @!                   | 37696354 |                1 |
|              0 | @$$                  | 21089863 |                1 |
|              0 | @&*!_Smilers         | 20573031 |                1 |
|              0 | @-Bristol            | 23905089 |                1 |
|              0 | @-symbol             | 25176688 |                1 |
|              0 | @440                 | 18159966 |                1 |
|              0 | @ANCAlerts           | 47685646 |                0 |
|              0 | @Andheri             | 42866631 |                0 |
|              0 | @Andheri_(2014_film) | 42914733 |                1 |
|              0 | @BarackObama         | 35944823 |                1 |
+----------------+----------------------+----------+------------------+
11 rows in set (0.09 sec)

This was fixed on 5.7: http://mysqlserverteam.com/mysql-5-7-only_full_group_by-improved-recognizing-functional-dependencies-enabled-by-default/ which given the other large issue we have with the optimizer we should seriously consider (I would have to check MariaDB 10.1).

My rationale is that 5.7, for external users, has this mode enabled by default, and we should be compatible with it. Enforcing it would be a second thought. But given the past experiences, I think it would fix more issues than it break things.

Let's give a chance to beta, which should be stricter than production, and not the other way round!

I have myself have no idea how to enable strict mode / which part of puppet config files have to be tweaked. The beta cluster database are also still running on Precise instance and MariaDB 5.5 , we might want to migrate them to migrate them to Jessie and MariaDB 5.10 first (that is T138778)

This is not blocked by that- it only needs to be enabled on config (see my comment on the child ticket).

Thanks @Huji for noting the SecurePoll strict mode problems (T147875).

I'll note this task still seems to be unassigned. @hashar, is the Beta cluster running in strict mode?

The beta cluster databases have been migrated to Jessie / MariaDB 5.10 T138778 and their config file is https://github.com/wikimedia/operations-puppet/blob/d3a05afd154/templates/mariadb/beta.my.cnf.erb

On beta I would guess it is about setting $wgSQLMode via operations/mediawiki-config , maybe that already has been done. For CI the task is T119371.

I am lurking on this task but reasonably have zero free time to lead the effort.

https://gerrit.wikimedia.org/r/#/c/429386/ would enable sql_mode = TRADITIONAL via includes/DevelopmentSettings.php which is loaded by CI. Tracked by sub task T119371.

The need from Wikimedia-Rdbms was to add the option and any additional support. I believe that part is done. (Ideally that would've been a subtask, but alas, better next time.)