ddl, planner, config: report unsupported for create/drop temporary table #14452

ffutop · 2020-01-11T11:36:22Z

What problem does this PR solve?

Fix pingcap/parser#609
report unsupported error.

What is changed and how it works?

# Turn off option ignore-ddl-temporary-keyword
mysql> CREATE TEMPORARY TABLE t (a INT);
ERROR 8200 (HY000): 'CREATE TEMPORARY TABLE' is currently unsupported
mysql> DROP TEMPORARY TABLE t;
ERROR 8200 (HY000): 'DROP TEMPORARY TABLE' is currently unsupported
mysql> CREATE TABLE t (a INT);
Query OK, 0 rows affected (0.01 sec)

mysql> DROP TABLE t;
Query OK, 0 rows affected (0.01 sec)

mysql> CREATE TEMPORARY SEQUENCE seq START WITH 100;
ERROR 8200 (HY000): 'CREATE TEMPORARY SEQUENCE' is currently unsupported
mysql> DROP TEMPORARY SEQUENCE seq;
ERROR 8200 (HY000): 'DROP TEMPORARY SEQUENCE' is currently unsupported
mysql> CREATE SEQUENCE seq START WITH 100;
Query OK, 0 rows affected (0.00 sec)

mysql> DROP SEQUENCE seq;
Query OK, 0 rows affected (0.00 sec)

# Turn on option ignore-ddl-temporary-keyword (default)
mysql> CREATE TEMPORARY TABLE t (a INT);
Query OK, 0 rows affected, 1 warning (0.01 sec)

mysql> SHOW CREATE TABLE t\G
*************************** 1. row ***************************
       Table: t
Create Table: CREATE TABLE `t` (
  `a` int(11) DEFAULT NULL
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.00 sec)

mysql> DROP TEMPORARY TABLE t;
Query OK, 0 rows affected, 1 warning (0.01 sec)

mysql> SHOW TABLES;
Empty set (0.00 sec)

mysql> CREATE TEMPORARY SEQUENCE seq START WITH 100;
Query OK, 0 rows affected, 1 warning (0.00 sec)

mysql> SHOW CREATE TABLE seq\G
*************************** 1. row ***************************
       Table: seq
Create Table: CREATE TABLE `seq` (

) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.00 sec)

mysql> DROP TEMPORARY SEQUENCE seq;
Query OK, 0 rows affected, 1 warning (0.00 sec)

Check List

Tests

Unit test

wwar · 2020-01-11T17:27:32Z

May I suggest an error message similar to "CREATE TEMPORARY TABLE is currently unsupported".

The message "Unsupported temporary table" is ambiguous; it could be interpreted as temporary tables are supported, but this particular one is not.

bb7133 · 2020-01-12T12:48:34Z

Hi @ffutop , thanks for your contribution!

bb7133 · 2020-01-14T01:49:34Z

The implementation looks fine for me. However, changing such behaviors means a potential breakdown for TiDB users. For example, many users switched their databases from MySQL to TiDB may have their scripts with TEMPORARY TABLE, that's why TiDB ignores this keyword by design.

Would you please add a configuration option(naming 'compatible mode', for example)? You can check the implementation of https://github.com/pingcap/tidb/blob/master/config/config.toml.example#L61 as a reference.

wwar · 2020-01-14T02:17:31Z

@bb7133 I think the behavior proposed here is an improvement since the case where the keyword TEMPORARY can be ignored is quite narrow:

The script would have to be single-threaded, and cleanup after itself to prevent name collisions.
There are also potential leakage scenarios, since the script may not have assumed that another user could read the data, or assume concurrent usage.
TEMPORARY tables also have a weird behavior where they can have the same name as a regular table, and reading from the temp table will take precedence.

ffutop · 2020-01-14T05:54:36Z

@bb7133 I'd like to take your advice. I'm not sure is it ok naming new option ignore-ddl-temporary-keywords

tangenta

Rest LGTM

planner/core/preprocess_test.go

tangenta

LGTM

bb7133 · 2020-01-15T02:25:43Z

@bb7133 I'd like to take your advice. I'm not sure is it ok naming new option ignore-ddl-temporary-keywords

How do you think about this option? @tangenta @kennytm

kennytm · 2020-01-15T03:17:10Z

Please also handle CREATE TEMPORARY SEQUENCE and DROP TEMPORARY SEQUENCE (if CREATE SEQUENCE already works)
Do we care about the CREATE TEMPORARY TABLES privilege
Having an option is fine. But are we going to keep this option for a long time (e.g. what would we do after we've actually implemented TEMPORARY TABLE)?

ffutop · 2020-01-15T12:19:28Z

@kennytm I can not find any mysql docs that introduce CREATE/DROP SEQUENCE.
And I tried following commands based on parser.y

CREATE SEQUENCE create a new table. And DROP SEQUENCE not works.

tidb> CREATE SEQUENCE seq;
Query OK, 0 rows affected (0.00 sec)

tidb> SHOW TABLES;
+----------------+
| Tables_in_test |
+----------------+
| seq            |
+----------------+
1 row in set (0.00 sec)

tidb> DROP SEQUENCE seq;
Query OK, 0 rows affected (0.00 sec)

tidb> SHOW TABLES;
+----------------+
| Tables_in_test |
+----------------+
| seq            |
+----------------+
1 row in set (0.00 sec)

tidb> DROP TABLE seq;
Query OK, 0 rows affected (0.01 sec)

tidb> SHOW TABLES;
Empty set (0.00 sec)

kennytm · 2020-01-15T15:15:42Z

@ffutop You could refer to MariaDB's documentation https://mariadb.com/kb/en/create-sequence/ for the syntax (this mostly equivalent to the "sequence generator" feature in ANSI SQL:2003, also shared by PostgreSQL, Oracle Database, Microsoft Transact-SQL, IBM DB2, and probably many other RDBMS).

Anyway, my comment is that CREATE TEMPORARY SEQUENCE seq ... should have the same treatment as CREATE TEMPORARY TABLE tbl (...) regarding the "TEMPORARY" keyword. Since this feature was recently implemented I wasn't sure if it already works on latest TiDB, and it seems to be working.

bb7133 · 2020-01-16T02:52:35Z

I can not find any mysql docs that introduce CREATE/DROP SEQUENCE.

The support of SEQUENCE is in-progress(although it is not supported by MySQL, as what you find out already).

…d-for-temporary-table # Conflicts: # config/config.toml.example # config/config_test.go

ffutop · 2020-01-19T12:35:49Z

@bb7133 PTAL

sre-bot · 2020-03-09T01:51:44Z

@ffutop, please update your pull request.

…for-temporary-table # Conflicts: # config/config.go # config/config.toml.example # config/config_test.go # planner/core/preprocess.go # planner/core/preprocess_test.go

…d-for-temporary-table # Conflicts: # ddl/ddl.go

codecov · 2020-03-11T12:40:36Z

Codecov Report

Merging #14452 into master will not change coverage by %.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #14452   +/-   ##
===========================================
  Coverage   80.6165%   80.6165%           
===========================================
  Files           505        505           
  Lines        136142     136142           
===========================================
  Hits         109753     109753           
  Misses        17892      17892           
  Partials       8497       8497

zz-jason · 2020-03-15T02:03:21Z

config/config.toml.example

+# ignore-ddl-temporary-keyword is used to make ddl TEMPORARY keyword compatible with previous version TiDB.
+# turn on this option, 'TEMPORARY' will ignored by TiDB as previous. Please make sure you fully understand the negative impact.
+# turn off this option, use 'CREATE/DROP TEMPORARY TABLE' or 'CREATE/DROP TEMPORARY SEQUENCE' will reported an error.
+ignore-ddl-temporary-keyword = false


Consider an older version of TiDB, which do not have this config item in the config file, updated to this version. The default value of this item is false, then there is chance that the application may failed to run on the newer version TiDB.

BTW, it makes user confused when we using double negation. How about renaming it to reserve-ddl-temporary-keyword?

OK, I will change the defaultConf.IgnoreDDLTemporaryKeyword to true.

but, I cannot agree to your renaming advice. reserve-ddl-temporary-keyword is ambiguous compare with ignore-xxx for new user, TEMPORARY is standard SQL syntax, why need to reserve? set to false might be interpreted as

no TEMPORARY syntax supported for tidb, or

just ignore TEMPORARY

…for-temporary-table

sre-bot · 2020-03-24T04:04:59Z

@ffutop, please update your pull request.

bb7133 · 2020-03-30T09:35:45Z

Hi @ffutop , please update the PR, thank you!

sre-bot · 2020-03-31T04:03:40Z

@ffutop, please update your pull request.

…for-temporary-table # Conflicts: # config/config.go # ddl/error.go

sre-bot · 2020-04-08T01:56:53Z

@ffutop, please update your pull request.

sre-bot · 2020-04-15T01:55:29Z

@ffutop, please update your pull request.

sre-bot · 2020-05-01T01:55:24Z

@ffutop PR closed due to no update for a long time. Feel free to reopen it anytime.

ffutop added 2 commits January 11, 2020 16:49

report unsupported for temporary table.

6da9ff2

add test.

69086e4

ffutop requested a review from a team as a code owner January 11, 2020 11:36

sre-bot added the contribution This PR is from a community contributor. label Jan 11, 2020

ghost requested review from alivxxx and winoros and removed request for a team January 11, 2020 11:36

add comment for exported var.

256782d

more explicit error report.

4092078

bb7133 added the type/enhancement The issue or PR belongs to an enhancement. label Jan 12, 2020

tangenta mentioned this pull request Jan 13, 2020

Call for participation: fix TiDB behaviors for incompatible statements pingcap/parser#598

Closed

11 tasks

tangenta reviewed Jan 14, 2020

View reviewed changes

planner/core/preprocess_test.go Outdated Show resolved Hide resolved

planner/core/preprocess_test.go Outdated Show resolved Hide resolved

correcting test cases.

742c7a3

tangenta reviewed Jan 14, 2020

View reviewed changes

tangenta added the status/LGT1 Indicates that a PR has LGTM 1. label Jan 14, 2020

ffutop changed the title ~~Report unsupported for create/drop temporary table~~ ddl: report unsupported for create/drop temporary table Jan 16, 2020

ffutop added 4 commits January 17, 2020 19:57

use a more generic error name.

752bd01

handle temporary keyword of CREATE/DROP SEQUENCE statement.

70694ce

option for ignore ddl temporary keyword

7de79dc

update example config

df19c2c

alivxxx removed their request for review January 19, 2020 03:44

Merge remote-tracking branch 'upstream/master' into report-unsupporte…

cbacbf5

…d-for-temporary-table # Conflicts: # config/config.toml.example # config/config_test.go

zz-jason added the component/DDL-need-LGT3 label Feb 3, 2020

zz-jason added component/DDL and removed component/DDL-need-LGT3 labels Feb 27, 2020

zz-jason changed the title ~~ddl: report unsupported for create/drop temporary table~~ ddl, planner, config: report unsupported for create/drop temporary table Mar 2, 2020

zimulala added sig/sql-infra SIG: SQL Infra and removed component/DDL1 labels Mar 4, 2020

Merge remote-tracking branch 'origin/master' into report-unsupported-…

7a4153d

…for-temporary-table # Conflicts: # config/config.go # config/config.toml.example # config/config_test.go # planner/core/preprocess.go # planner/core/preprocess_test.go

github-actions bot added the component/config label Mar 10, 2020

ffutop added 2 commits March 10, 2020 21:16

add comments

da4d695

Merge remote-tracking branch 'upstream/master' into report-unsupporte…

8d77a1a

…d-for-temporary-table # Conflicts: # ddl/ddl.go

zz-jason requested review from zimulala and bb7133 March 15, 2020 01:57

zz-jason reviewed Mar 15, 2020

View reviewed changes

ffutop added 3 commits March 17, 2020 12:55

set default ignore-ddl-temporary-keyword to true for compatibility

94afe18

fix DATA RACE

d8c3352

Merge remote-tracking branch 'origin/master' into report-unsupported-…

75de0f2

…for-temporary-table

Merge remote-tracking branch 'origin/master' into report-unsupported-…

e4a1b7a

…for-temporary-table # Conflicts: # config/config.go # ddl/error.go

sre-bot closed this May 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ddl, planner, config: report unsupported for create/drop temporary table #14452

ddl, planner, config: report unsupported for create/drop temporary table #14452

ffutop commented Jan 11, 2020 •

edited

Loading

wwar commented Jan 11, 2020

bb7133 commented Jan 12, 2020

bb7133 commented Jan 14, 2020

wwar commented Jan 14, 2020

ffutop commented Jan 14, 2020

tangenta left a comment

tangenta left a comment

bb7133 commented Jan 15, 2020

kennytm commented Jan 15, 2020 •

edited

Loading

ffutop commented Jan 15, 2020

kennytm commented Jan 15, 2020

bb7133 commented Jan 16, 2020

ffutop commented Jan 19, 2020

sre-bot commented Mar 9, 2020

codecov bot commented Mar 11, 2020 •

edited

Loading

zz-jason Mar 15, 2020

ffutop Mar 15, 2020

sre-bot commented Mar 24, 2020

bb7133 commented Mar 30, 2020

sre-bot commented Mar 31, 2020

sre-bot commented Apr 8, 2020

sre-bot commented Apr 15, 2020

sre-bot commented May 1, 2020

ddl, planner, config: report unsupported for create/drop temporary table #14452

ddl, planner, config: report unsupported for create/drop temporary table #14452

Conversation

ffutop commented Jan 11, 2020 • edited Loading

What problem does this PR solve?

What is changed and how it works?

Check List

wwar commented Jan 11, 2020

bb7133 commented Jan 12, 2020

bb7133 commented Jan 14, 2020

wwar commented Jan 14, 2020

ffutop commented Jan 14, 2020

tangenta left a comment

Choose a reason for hiding this comment

tangenta left a comment

Choose a reason for hiding this comment

bb7133 commented Jan 15, 2020

kennytm commented Jan 15, 2020 • edited Loading

ffutop commented Jan 15, 2020

kennytm commented Jan 15, 2020

bb7133 commented Jan 16, 2020

ffutop commented Jan 19, 2020

sre-bot commented Mar 9, 2020

codecov bot commented Mar 11, 2020 • edited Loading

Codecov Report

zz-jason Mar 15, 2020

Choose a reason for hiding this comment

ffutop Mar 15, 2020

Choose a reason for hiding this comment

sre-bot commented Mar 24, 2020

bb7133 commented Mar 30, 2020

sre-bot commented Mar 31, 2020

sre-bot commented Apr 8, 2020

sre-bot commented Apr 15, 2020

sre-bot commented May 1, 2020

ffutop commented Jan 11, 2020 •

edited

Loading

kennytm commented Jan 15, 2020 •

edited

Loading

codecov bot commented Mar 11, 2020 •

edited

Loading