ddl: Add `sql_require_primary_key` sysvar by joycse06 · Pull Request #36146 · pingcap/tidb

joycse06 · 2022-07-12T10:48:04Z

What problem does this PR solve?

Issue Number: close #28544

Problem Summary:

This is a feature from MySQL 8.0:
https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_sql_require_primary_key

It allows you to enforce tables have a primary key. In MySQL it is important because Row-based replication benefits from it.

With TiDB, TiCDC (mysql sink) will not replicate tables without a primary/unique key by default. If you chose to replicate tables without a primary key, the data will be duplicated. For our use-case this is an issue, since we use TiCDC to async replicate between clusters we intend to failover to.

What is changed and how it works?

sql_require_primary_key sysvar has been added. When enabled it will throw an error if a create table ddl doesn't have a primary key.

Check List

Tests

Unit test
Integration test
Manual test (add detailed scripts or steps below)
No code

Side effects

Performance regression: Consumes more CPU
Performance regression: Consumes more Memory
Breaking backward compatibility

Documentation

Release note

Please refer to Release Notes Language Style Guide to write a quality release note.

The system variable `sql_require_primary_key` has been added. This behaves the same as the feature in MySQL 8.0, and when enabled tables can not be created without primary keys.

ti-chi-bot · 2022-07-12T10:48:05Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

dveeden
morgo

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.

Details

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

sre-bot · 2022-07-12T10:58:36Z

Code Coverage Details: https://codecov.io/github/pingcap/tidb/commit/8cf6e3611707ab50e65373cd62e6174250ae913b

joycse06 · 2022-07-12T11:49:09Z

/run-unit-test
/run-check_dev_2

dveeden · 2022-07-12T12:51:38Z

sql> SET sql_require_primary_key=ON;
Query OK, 0 rows affected (0.0007 sec)

sql> CREATE TABLE t1(id int primary key);
Query OK, 0 rows affected (0.0076 sec)

sql> CREATE TABLE t2(id SERIAL);
ERROR: 1173 (42000): This table type requires a primary key%!(EXTRA model.CIStr=t2)

sql> SET sql_require_primary_key=OFF;
Query OK, 0 rows affected (0.0007 sec)

sql> CREATE TABLE t2(id SERIAL);
Query OK, 0 rows affected (0.0091 sec)

sql> SHOW CREATE TABLE t2\G
*************************** 1. row ***************************
       Table: t2
Create Table: CREATE TABLE `t2` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT,
  UNIQUE KEY `id` (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.0011 sec)

I think a unique key instead of a PK should be allowed. However I don't think this is common or advisable, so this is a very minor thing.

dveeden · 2022-07-12T12:56:55Z

I don't think this should be allowed:

sql> SET SESSION sql_require_primary_key=ON;
Query OK, 0 rows affected (0.0007 sec)

sql> SHOW CREATE TABLE t4\G
*************************** 1. row ***************************
       Table: t4
Create Table: CREATE TABLE `t4` (
  `id` int(11) NOT NULL,
  `c1` int(11) DEFAULT NULL,
  PRIMARY KEY (`id`) /*T![clustered_index] NONCLUSTERED */
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.0010 sec)

sql> ALTER TABLE t4 DROP PRIMARY KEY;
Query OK, 0 rows affected (0.0222 sec)

dveeden · 2022-07-12T12:57:51Z

/cc @dveeden

dveeden · 2022-07-12T13:06:53Z

Not sure if this should apply to temp tables:

sql> CREATE TEMPORARY TABLE t5 (id int);
ERROR: 1173 (42000): This table type requires a primary key%!(EXTRA model.CIStr=t5)

Another thing to check/test for is test for upgrades, internal tables, etc. as that caused some issues for MySQL when this was introduced.

7yyo · 2022-07-12T13:13:08Z

https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_sql_require_primary_key

It doesn't just work on create table

morgo · 2022-07-12T15:06:37Z

https://dev.mysql.com/doc/refman/8.0/en/server-system-variables.html#sysvar_sql_require_primary_key

It doesn't just work on create table

These alter tables probably are not supported in TiDB (ADD/DROP PRIMARY KEY is deprecated in TiDB; but it looks like it might work on non-clustered tables so we will need to test this.)

Another thing to check/test for is test for upgrades, internal tables, etc. as that caused some issues for MySQL when this was introduced.

Good point! I suggest we always enable sql_require_primary_key for the bootstrap session. I can't imagine we require non-PK tables in the internal dictionary and that will get us out of settings compatibility trouble.

sql> CREATE TABLE t2(id SERIAL);
ERROR: 1173 (42000): This table type requires a primary key%!(EXTRA model.CIStr=t2)

I think this might be a bug somewhere. The code that does tbInfo.GetPkName() is supposed to consider the InnoDB rules for promoting unique indexes to "PK", which should apply here.

morgo · 2022-07-12T15:08:03Z

Getting a linter error:

[2022-07-12T11:52:36.931Z]   ✘  https://revive.run/r#var-naming  const SqlRequirePrimaryKey should be SQLRequirePrimaryKey  
[2022-07-12T11:52:36.932Z]   sessionctx/variable/sysvar.go:1987:2

You may also need to run make errdoc to update the errors file.

Co-authored-by: Morgan Tocker <tocker@gmail.com>

… key

joycse06 · 2022-07-13T01:29:13Z

Not sure if this should apply to temp tables:

MySQL 8 applies it to temporary tables as well.

From the doc:

sql_require_primary_key applies to both base tables and TEMPORARY tables

joycse06 · 2022-07-13T01:32:39Z

I wasn't using the correct error code but a similar one. I have now added the actual error code. Have also added check to stop dropping Primary Key indexes (non-clustered one's) when this is set.

tidb> set sql_require_primary_key = 1;
Query OK, 0 rows affected (0.00 sec)

tidb> SHOW VARIABLES LIKE 'sql_require_primary_key';
+-------------------------+-------+
| Variable_name           | Value |
+-------------------------+-------+
| sql_require_primary_key | ON    |
+-------------------------+-------+
1 row in set (0.00 sec)

tidb> CREATE TABLE `t4` (`id` int(11) NOT NULL, `c1` int(11) DEFAULT NULL);
ERROR 3750 (HY000): Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set. Add a primary key to the table or unset this variable to avoid this message. Note that tables without a primary key can cause performance problems in row-based replication, so please consult your DBA before changing this setting.
tidb> CREATE TABLE `t4` (`id` int(11) NOT NULL, `c1` int(11) DEFAULT NULL, PRIMARY KEY (`id`));
Query OK, 0 rows affected (0.01 sec)

tidb> ALTER TABLE t4 DROP COLUMN id;
ERROR 8200 (HY000): Unsupported drop integer primary key
tidb> ALTER TABLE t4 DROP PRIMARY KEY;
ERROR 8200 (HY000): Unsupported drop primary key when the table's pkIsHandle is true
tidb> DROP TABLE t4;
Query OK, 0 rows affected (0.01 sec)

tidb> CREATE TABLE `t4` (`id` int(11) NOT NULL, `c1` int(11) DEFAULT NULL, PRIMARY KEY (`id`) NONCLUSTERED);
Query OK, 0 rows affected (0.00 sec)

tidb> ALTER TABLE t4 DROP COLUMN id;
ERROR 8200 (HY000): can't drop column id with composite index covered or Primary Key covered now
tidb> ALTER TABLE t4 DROP PRIMARY KEY;
ERROR 3750 (HY000): Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set. Add a primary key to the table or unset this variable to avoid this message. Note that tables without a primary key can cause performance problems in row-based replication, so please consult your DBA before changing this setting.

For some cases TiDB is returning ERROR 8200 instead of ERROR 3750 for thats coming from existing code, I am not sure if I should change those.

joycse06 · 2022-07-13T01:34:34Z


+["schema:3750"]
+error = '''
+Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set. Add a primary key to the table or unset this variable to avoid this message. Note that tables without a primary key can cause performance problems in row-based replication, so please consult your DBA before changing this setting.


This is the same error mysql returns, copied from here

dveeden · 2022-07-13T08:30:24Z

ERROR 3750 (HY000): Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set. Add a primary key to the table or unset this variable to avoid this message. Note that tables without a primary key can cause performance problems in row-based replication, so please consult your DBA before changing this setting.

The "...row-based replication..." part is a bit weird in a TiDB context, we could change this to:

ERROR 3750 (HY000): Unable to create or change a table without a primary key, when the system variable 'sql_require_primary_key' is set. Add a primary key to the table or unset this variable to avoid this message.

But keeping this message identical to MySQL has my personal preference.

dveeden · 2022-07-13T10:20:21Z

/cc @7yyo

7yyo

LGTM

ti-chi-bot · 2022-07-13T12:37:38Z

@7yyo: Thanks for your review. The bot only counts approvals from reviewers and higher roles in list, but you're still welcome to leave your comments.

Details

In response to this:

LGTM

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.

dveeden · 2022-07-15T07:29:08Z

Note that DM doesn't use real PKs either:

sql> show create table dm_meta.`demo-mysql8-tidb_loader_checkpoint`\G
*************************** 1. row ***************************
       Table: demo-mysql8-tidb_loader_checkpoint
Create Table: CREATE TABLE `demo-mysql8-tidb_loader_checkpoint` (
  `id` char(32) NOT NULL,
  `filename` varchar(255) NOT NULL,
  `cp_schema` varchar(128) NOT NULL,
  `cp_table` varchar(128) NOT NULL,
  `offset` bigint(20) NOT NULL,
  `end_pos` bigint(20) NOT NULL,
  `create_time` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
  `update_time` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  UNIQUE KEY `uk_id_f` (`id`,`filename`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.0005 sec)

sql> show create table dm_meta.`demo-mysql8-tidb_syncer_checkpoint`\G
*************************** 1. row ***************************
       Table: demo-mysql8-tidb_syncer_checkpoint
Create Table: CREATE TABLE `demo-mysql8-tidb_syncer_checkpoint` (
  `id` varchar(32) NOT NULL,
  `cp_schema` varchar(128) NOT NULL,
  `cp_table` varchar(128) NOT NULL,
  `binlog_name` varchar(128) DEFAULT NULL,
  `binlog_pos` int(10) unsigned DEFAULT NULL,
  `binlog_gtid` text DEFAULT NULL,
  `exit_safe_binlog_name` varchar(128) DEFAULT '',
  `exit_safe_binlog_pos` int(10) unsigned DEFAULT '0',
  `exit_safe_binlog_gtid` text DEFAULT NULL,
  `table_info` json NOT NULL,
  `is_global` tinyint(1) DEFAULT NULL,
  `create_time` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP,
  `update_time` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,
  UNIQUE KEY `uk_id_schema_table` (`id`,`cp_schema`,`cp_table`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_bin
1 row in set (0.0016 sec)

This is with DM v5.4.2. I assume the unique keys with all columns defined as NOT NULL should be seen as good-enough.

bb7133 · 2022-07-21T03:07:30Z

Hi @joycse06 , thanks for the contribution!

If this variable is supposed to be used with TiCDC-based replication, should we consider unique key as well?

I know that unique key is not included in sql_require_primary_key for MySQL, is it better to introduce another system variables for both primary key and unique key check?

morgo · 2022-07-22T13:23:53Z

If this variable is supposed to be used with TiCDC-based replication, should we consider unique key as well?

We don't have a use-case beyond what the MySQL feature offers. It's not a typical use-case to have a table which doesn't have a primary key, but has a unique key.

…rimary-key * upstream/master: (104 commits) br: fix compatibility issue with concurrent ddl (pingcap#36474) infoschema: fix PromQL for `tidb_distsql_copr_cache` (pingcap#36450) test: stabilize TestTopSQLCPUProfile (pingcap#36468) parser: add support of 'ADMIN SHOW DDL JOB QUERIES LIMIT m OFFSET n' transferring to AST (pingcap#36285) *: enable flaky test for all test (pingcap#36385) expression: fix return type of agg func `bit_or` when handling varbinary column (pingcap#36415) executor: fix aggregating enum zero value gets different results from mysql (pingcap#36208) server: skip check tiflash version (pingcap#36451) *: Minor update to SECURITY.md to improved clarity (pingcap#36346) table partition: add telemetry for partition table (pingcap#36204) ddl: invalid multiple MAXVALUE partitions (pingcap#36329) (pingcap#36345) planner: Fixed `Merge` hint in nested CTE (pingcap#36432) metric: impove concurrency ddl metrics (pingcap#36405) planner: add more test cases for leading outer join (pingcap#36409) ddl: only set concurrent variable if no error (pingcap#36437) ddl: fix update panic in the middle of multi-schema change (pingcap#36421) session: Mising OptimizeWithPlanAndThenWarmUp in prepare-execute path (pingcap#36347) executor,metrics: add a metric for observing execution phases (pingcap#35906) br: unified docker image align with tidb (pingcap#36016) ddl: skip to close nil sessPool (pingcap#36425) ...

joycse06 · 2022-07-23T16:12:00Z

If this variable is supposed to be used with TiCDC-based replication, should we consider unique key as well?

@bb7133 I agree with Morgan on this. As we don't have a compelling use case to go beyond what MySQL offers atm, I think we should have it do what MySQL does.

bb7133

LGTM, please fix the lint as commented by @morgo: #36146 (comment)

bb7133 · 2022-08-03T08:34:25Z

/merge

ti-chi-bot · 2022-08-03T08:34:30Z

This pull request has been accepted and is ready to merge.

Details

Commit hash: 4eb5856

sre-bot · 2022-08-03T10:03:07Z

TiDB MergeCI notify

🔴 Bad News! [1] CI still failing after this pr merged.
These failed integration tests don't seem to be introduced by the current PR.

CI Name	Result	Duration	Compare with Parent commit
idc-jenkins-ci-tidb/integration-common-test	🔴 failed 3, success 14, total 17	22 min	Existing failure
idc-jenkins-ci/integration-cdc-test	🟢 all 36 tests passed	34 min	Existing passed
idc-jenkins-ci-tidb/common-test	🟢 all 11 tests passed	14 min	Existing passed
idc-jenkins-ci-tidb/integration-ddl-test	🟢 all 6 tests passed	10 min	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-2	🟢 all 28 tests passed	9 min 59 sec	Existing passed
idc-jenkins-ci-tidb/tics-test	🟢 all 1 tests passed	7 min 55 sec	Existing passed
idc-jenkins-ci-tidb/sqllogic-test-1	🟢 all 26 tests passed	7 min 25 sec	Existing passed
idc-jenkins-ci-tidb/mybatis-test	🟢 all 1 tests passed	4 min 35 sec	Existing passed
idc-jenkins-ci-tidb/integration-compatibility-test	🟢 all 1 tests passed	4 min 13 sec	Existing passed
idc-jenkins-ci-tidb/plugin-test	🟢 build success, plugin test success	4min	Existing passed

bb7133 · 2022-08-03T13:05:28Z

Hi @joycse06 , thanks for your contribution! Would you like to add the document for this variable(https://github.com/pingcap/docs/blob/master/system-variables.md)?

joycse06 · 2022-08-08T00:12:31Z

@bb7133 Sure, I will give it a go. I will raise a PR in that repo when ready. Thanks.

joycse06 · 2022-08-09T03:44:29Z

@bb7133 Can you please have a look at pingcap/docs#9886 ? Thanks

Add sql_require_primary_key sys var

b3a6af1

joycse06 requested a review from a team as a code owner July 12, 2022 10:48

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 12, 2022

ti-chi-bot requested a review from dveeden July 12, 2022 12:57

morgo reviewed Jul 12, 2022

View reviewed changes

Comment thread ddl/ddl_api.go Outdated

joycse06 and others added 4 commits July 13, 2022 07:51

Make the error message static

a903fb2

Co-authored-by: Morgan Tocker <tocker@gmail.com>

Use correct error code from MySQL and add guard while droping primary…

7aa65ab

… key

Merge branch 'master' into add-sql-require-primary-key

094cd7f

Fix linter error and run make errdoc

2f35ee8

joycse06 commented Jul 13, 2022

View reviewed changes

dveeden approved these changes Jul 13, 2022

View reviewed changes

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 13, 2022

ti-chi-bot requested a review from 7yyo July 13, 2022 10:20

7yyo approved these changes Jul 13, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2022

ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 23, 2022

joycse06 commented Jul 23, 2022

View reviewed changes

Comment thread parser/parser.go Outdated

joycse06 and others added 2 commits July 25, 2022 09:53

Merge branch 'master' into add-sql-require-primary-key

7829df4

Merge branch 'master' into add-sql-require-primary-key

25e1df0

bb7133 approved these changes Jul 31, 2022

View reviewed changes

joycse06 and others added 2 commits August 3, 2022 08:03

Merge branch 'master' into add-sql-require-primary-key

a07c990

Revert changes in generated files

d3c184e

joycse06 force-pushed the add-sql-require-primary-key branch from 9e9038b to d3c184e Compare August 3, 2022 00:13

Fix spec

4eb5856

joycse06 force-pushed the add-sql-require-primary-key branch from 2f60169 to 4eb5856 Compare August 3, 2022 01:11

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Aug 3, 2022

ti-chi-bot added 3 commits August 3, 2022 16:34

Merge branch 'master' into add-sql-require-primary-key

54e1044

Merge branch 'master' into add-sql-require-primary-key

d09e813

Merge branch 'master' into add-sql-require-primary-key

8cf6e36

ti-chi-bot merged commit 6a6fa8a into pingcap:master Aug 3, 2022

joycse06 mentioned this pull request Aug 9, 2022

Document require_primary_key system variable pingcap/docs#9886

Merged

13 tasks

Conversation

joycse06 commented Jul 12, 2022 • edited by morgo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What problem does this PR solve?

What is changed and how it works?

Check List

Release note

Uh oh!

ti-chi-bot commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sre-bot commented Jul 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joycse06 commented Jul 12, 2022

Uh oh!

dveeden commented Jul 12, 2022

Uh oh!

dveeden commented Jul 12, 2022

Uh oh!

dveeden commented Jul 12, 2022

Uh oh!

dveeden commented Jul 12, 2022

Uh oh!

7yyo commented Jul 12, 2022

Uh oh!

Uh oh!

morgo commented Jul 12, 2022

Uh oh!

morgo commented Jul 12, 2022

Uh oh!

joycse06 commented Jul 13, 2022

Uh oh!

joycse06 commented Jul 13, 2022

Uh oh!

joycse06 Jul 13, 2022

Choose a reason for hiding this comment

Uh oh!

dveeden commented Jul 13, 2022

Uh oh!

dveeden commented Jul 13, 2022

Uh oh!

7yyo left a comment

Choose a reason for hiding this comment

Uh oh!

ti-chi-bot commented Jul 13, 2022

Uh oh!

dveeden commented Jul 15, 2022

Uh oh!

bb7133 commented Jul 21, 2022

Uh oh!

morgo commented Jul 22, 2022

Uh oh!

joycse06 commented Jul 23, 2022

Uh oh!

Uh oh!

bb7133 left a comment

Choose a reason for hiding this comment

Uh oh!

bb7133 commented Aug 3, 2022

Uh oh!

ti-chi-bot commented Aug 3, 2022

Uh oh!

sre-bot commented Aug 3, 2022

TiDB MergeCI notify

Uh oh!

bb7133 commented Aug 3, 2022

Uh oh!

joycse06 commented Aug 8, 2022

Uh oh!

joycse06 commented Aug 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

joycse06 commented Jul 12, 2022 •

edited by morgo

Loading

ti-chi-bot commented Jul 12, 2022 •

edited

Loading

sre-bot commented Jul 12, 2022 •

edited

Loading