planner: include "CREATE/DROP TEMPORARY TABLE" to noop functions (##609)

[nayuta-yanagisawa]

What problem does this PR solve?

Issue Number: close pingcap/parser#609

Problem Summary:

Currently, TiDB ignores the TEMPORARY keyword in "CREATE/DROP TEMPORARY TABLE" statements.

What is changed and how it works?

I added "CREATE/DROP TEMPORARY TABLE" statements to noop functions. The statements returns errors when and only when tidb_enable_noop_functions = OFF.

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

mysql> CREATE TEMPORARY TABLE t (a INT);
ERROR 1235 (42000): function CREATE TEMPORARY TABLE has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions
mysql> DROP TEMPORARY TABLE t;
ERROR 1235 (42000): function DROP TEMPORARY TABLE has only noop implementation in tidb now, use tidb_enable_noop_functions to enable these functions
mysql> set tidb_enable_noop_functions=1;
Query OK, 0 rows affected (0.00 sec)

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

mysql> CREATE TEMPORARY TABLE t (a INT);
Query OK, 0 rows affected, 1 warning (0.07 sec)

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

mysql> show warnings;
+---------+------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Level   | Code | Message                                                                                                                                                                                                                                      |
+---------+------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Warning | 1064 | You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 22 near "TABLE t (a INT)"TiDB doesn't support TEMPORARY TABLE, TEMPORARY will be parsed but ignored.  |
+---------+------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+

Note that "CREATE/DROP TEMPORARY SEQUENCE" statements are out of the scope of the pull request. TiDB seems not to support the statements as far as I read the documentation, TiDB SEQUENCE GENERATOR. In fact, TiDB v4.0 returns syntax errors for the statements.

mysql> \s
--------------
mysql  Ver 8.0.23-0ubuntu0.20.04.1 for Linux on x86_64 ((Ubuntu))

Connection id:          4
Current database:
Current user:           [email protected]
SSL:                    Not in use
Current pager:          stdout
Using outfile:          ''
Using delimiter:        ;
Server version:         5.7.25-TiDB-v4.0.0 TiDB Server (Apache License 2.0) Community Edition, MySQL 5.7 compatible
Protocol version:       10
Connection:             127.0.0.1 via TCP/IP
Server characterset:    utf8mb4
Db     characterset:    utf8mb4
Client characterset:    utf8mb4
Conn.  characterset:    utf8mb4
TCP port:               4000
Binary data as:         Hexadecimal
--------------

mysql> CREATE TEMPORARY SEQUENCE seq START WITH 100;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 25 near "SEQUENCE seq START WITH 100"

mysql> DROP TEMPORARY SEQUENCE seq;
ERROR 1064 (42000): You have an error in your SQL syntax; check the manual that corresponds to your TiDB version for the right syntax to use line 1 column 23 near "SEQUENCE seq"

Related changes

• doc: Update compatibility for temporary tables in dev docs#4935

Check List

Tests

• Unit test

Side effects

• Breaking backward compatibility

Release note

• Let "CREATE/DROP TEMPORARY TABLE" return errors if tidb_enable_noop_functions = OFF.

[sre-bot]

No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md

[nayuta-yanagisawa]

Force pushed to fix the commit message.

[morgo]

LGTM

[nayuta-yanagisawa]

The following documents should be updated once the present pull request is merged.

• https://github.com/pingcap/docs/blob/master/sql-statements/sql-statement-create-table.md#mysql-compatibility
• https://github.com/pingcap/docs/blob/master/system-variables.md#tidb_enable_noop_functions-new-in-v40

[bb7133]

LGTM

[bb7133]

PTAL @zz-jason

[morgo]

For reviewers: note that TEMPORARY TABLES have very different semantics to regular tables. It is very unlikely that by parsing but ignoring the TEMPORARY keyword an app will function fine.

A longer explaination:

• Temporary tables can use the same name as regular tables. When querying, the temporary table namespace takes precedence.
• Temporary tables are per-session, and automatically cleaned up when the session ends. If a regular table is used, there will typically be an error trying to create the table because it already exists.
• (Relates to point above) Temporary tables are often used for the same use-case as CTEs (build a list of rows and then perform a followup operation on them). Because temporary tables are usually session scope, there are no concurrency risks. But in TiDB there will be, as all sessions can read/write to the temporary table.

This is a really useful improvement to prevent incorrect usage and likely data corruption.

[zz-jason]

LGTM

[ti-chi-bot]

@zz-jason: Please use /LGTM instead of LGTM when you want to approve the pull request by comment.
If you use the GitHub review feature, please approve the PR directly, the comment will not take effect in the GitHub review feature.
If you have any qustions please refer to lgtm command help or lgtm plugin design.

If you have approved this PR, please ignore this reply. This reply is being used as a temporary reply during the migration of the new bot and will be removed on April 1.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

[ti-chi-bot]

[REVIEW NOTIFICATION]

This pull request has been approved by:

• zz-jason

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

[ti-chi-bot]

@nayuta-yanagisawa: GitHub didn't allow me to assign the following users: committer.

Note that only pingcap members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @committer

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[morgo]

/LGTM

[morgo]

/merge

[ti-chi-bot]

@morgo: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

[ti-chi-bot]

@morgo: /merge in this pull request requires 2 /lgtm.

In response to this:

/merge

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

[morgo]

/lgtm

[morgo]

I removed the require-LGT3 tag because it looked like the bot was misbehaving. But now I think the previous lgtm's may have failed because of the missing sig.

@hi-rustin do you mind taking a look?

[morgo]

/run-all-tests

[morgo]

/run-e2e-test

[Rustin170506]

I removed the require-LGT3 tag because it looked like the bot was misbehaving.

What is the misbehaving you are talking about? I think everything was fine before the require-LGT3 was removed. Now the bot can't merge because the number of lgtms doesn't match the required lgtms (it may look like a bug, but it's actually because require-LGT3 was removed). You should now add back require-LGT3 and it will work fine.

[Rustin170506]

I removed the require-LGT3 tag because it looked like the bot was misbehaving.

What is the misbehaving you are talking about? I think everything was fine before the require-LGT3 was removed. Now the bot can't merge because the number of lgtms doesn't match the required lgtms (it may look like a bug, but it's actually because require-LGT3 was removed). You should now add back require-LGT3 and it will work fine.

I modified the code so that it also merges when the number of lgtm is greater than the requested number. (But this is rarely the case, except in the above case), because the bot will only respond and reply to the requested number, instead of growing lgtm number infinitely.

[Rustin170506]

/run-e2e-test

Also this test was broken last week, the robot will not require this test to pass.

[Rustin170506]

/assign @committer

Here committer does not mean someone called committer. you can /assign @morgo to request morgo to merge your PR. You can find all the committers at this link: https://prow.tidb.io/tichi/repos/pingcap/tidb/pulls/22860/owners

/assign @morgo