Able to use Update without where clause in sql_safe_updates mode.

[ramanich1]

Bug Report

Please answer these questions before submitting your issue. Thanks!

1. Minimal reproduce step (Required)

drop table if exists t1;
SET SESSION sql_safe_updates = 1;
CREATE TABLE t1 ( a INT );
INSERT INTO t1 VALUES (1), (2);
select @@sql_safe_updates;
update t1 set a = 5;

2. What did you expect to see? (Required)

mysql> select @@sql_safe_updates;
+--------------------+
| @@sql_safe_updates |
+--------------------+
|                  1 |
+--------------------+
1 row in set (0.00 sec)

mysql> update t1 set a = 5;
ERROR 1175 (HY000): You are using safe update mode and you tried to update a table without a WHERE that uses a KEY column. 

3. What did you see instead (Required)

mysql> select @@sql_safe_updates;
+--------------------+
| @@sql_safe_updates |
+--------------------+
|                  1 |
+--------------------+
1 row in set (0.00 sec)

mysql> update t1 set a = 5;
Query OK, 2 rows affected (0.00 sec)
Rows matched: 2  Changed: 2  Warnings: 0

4. What is your TiDB version? (Required)

| Release Version: v5.4.0-alpha-264-g6efa36df6
Edition: Community
Git Commit Hash: 6efa36df6cff325106f67ecfe3d79816ba37ca6a
Git Branch: master
UTC Build Time: 2021-11-29 16:57:51
GoVersion: go1.17.2
Race Enabled: false
TiKV Min Version: v3.0.0-60965b006877ca7234adaced7890d7b029ed1306
Check Table Before Drop: false |

[winoros]

Seems that we haven't supported this variable yet.
@bb7133

[winoros]

Mark it as a compatibility problem.

[morgo]

I think it has to count as a bug because it is a safety/data loss prevention feature. Because we added a noop TiDB 'advertises' that it supports it, but it infact does not.

@CbcWestwolf Do you mind taking a look at this? We have two choices when we encounter variables like this:

1. Make it read only. Example: sessionctx/variable: make lc_time_names read only #30084
2. Require a special setting (tidb_enable_noop_functions) to be enabled in order to change it. Example: executor, session, sessionctx/variable: add validate function for sysval sql_auto_is_null #28247

We usually select the strategy based on the likelihood it will affect applications. In this case, I think (1) is fine because this feature is mostly used by user-clients (like the MySQL cli) and not applications.

@mjonss @dveeden any second opinions on the best behavior?

[dveeden]

From mysql --help:

...
  -U, --safe-updates  Only allow UPDATE and DELETE that uses keys.
  -U, --i-am-a-dummy  Synonym for option --safe-updates, -U.
...
  --select-limit=#    Automatic limit for SELECT when using --safe-updates.
  --max-join-size=#   Automatic limit for rows in a join when using
                      --safe-updates.
...
safe-updates                      FALSE
i-am-a-dummy                      FALSE
...
select-limit                      1000
max-join-size                     1000000
...

For MySQL Workbench, see https://dev.mysql.com/doc/workbench/en/wb-preferences-sql-editor.html, which states:

---

Limit Rows

Enabled by default. Queries can sometimes return an excessive number of rows, which can heavily load the connection, and take time to display in MySQL Workbench. To prevent this, you can set a more moderate value here. This limit is defined by the Limit Rows Count option.

Limit Rows Count: [ 200 ]

Specify the maximum number of result rows to return.

---

and

---

Safe Updates (rejects UPDATEs and DELETEs with no restrictions)

Enabled by default. Prevents UPDATE and DELETE queries that lack a corresponding key in a WHERE clause, or lack a LIMIT clause, from executing. This option requires a MySQL server reconnection.

When selected, this preference makes it possible to catch UPDATE and DELETE statements with keys that are not used properly and that can probably accidentally change or delete a large number of rows.

---

iirc the row limit in MySQL Workbench is just adding LIMIT 0,200 to queries.

We should avoid this causing MySQL Workbench to fail on connect and/or query time if possible. Other (GUI) clients may do something similar.

Besides making the variable ready only or require a special setting we could also actually implement this feature.

So once we have a PR we should test with MySQL Workbech (MySQL Client doesn't use this option by default) and make sure we agree on the impact it may have.

[morgo]

@dveeden Great catch! If we use read_only (1) it will break workbench by default. I think that's a problem.

It is likely also a problem if tidb_enable_noop_functions needs to be enabled to use workbench, since that will permit too much unsafe syntax.

We can test this out, but we may indeed be required to implement this feature to avoid pitfalls.

[mjonss]

My view is that we should see this as a bug and not an enhancement. If we are trying to be MySQL compatible, the users should be able to rely on --i-am-a-dummy / --safe-updates / @@sql_safe_updates

[morgo]

@mjonss Thank you, I agree with your conclusion.

However just to be pragmatic, I'll try read_only and see if workbench generates any warnings or errors when its not able to change the value. If it manages to connect OK (a big if) then we at least have an interim solution that is lower risk and easier to implement.