Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

*: support MySQL compatible AUTO_INCREMENT #38449

Closed
wants to merge 35 commits into from

Conversation

tiancaiamao
Copy link
Contributor

What problem does this PR solve?

Issue Number: close #38442

Problem Summary:

What is changed and how it works?

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

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

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

None

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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 submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Oct 13, 2022
## Compatibility

The old implementation can still be kept, its performance is better and might meet some user's needs.
We can rename the syntax for it, such as `AUTO_INCREMENT_FAST` or `AUTO_INCREMENT_OLD` or `DEFICIT_AUTO_INCREMENT`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose the name 'AUTO_INCREMENT_CACHED' for the old implementation or 'CACHED_AUTO_INCREMENT'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we name this:

  • `AUTO_INCREMENT`` for the new fully MySQL/InnoDB compatible method
  • AUTO_INCREMENT_FAST for the old, more distributed autoid

Then when migrating, using ORMs, tools, etc people are likely to use the new MySQL compatible autoid instead of the more scalable distributed autoid while they might not need the new behavior.

Another way of doing this would be this:

SET tidb_autoid_mysql_compatible=ON;
CREATE TABLE t1 (id INT AUTO_INCREMENT PRIMARY KEY);

By using a session variable instead of a different keyword we allow the default to use the distributed autoid while only using the new MySQL compatibility when really needed. The session variable can be set on the connection or via a global variable, this makes it possible to run third-party applications that rely on the new MySQL compatible autoid without having to change the SQL queries.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've update this part.

Use AUTO_ID_CACHE 1 to make it compatible with both old TiDB and MySQL


## Proposal

This proposes that we introduce a centralized auto ID allocating service, the `AUTO_INCREMENT` ID is allocated from the centralized service and there is no caching mechanism on the TiDB layer, so the behaviour can be MySQL compatible.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this would be another service in a primary and a backup TiDB node? Which are the reasons for having it in TiDB vs using the primary PD node for this service?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now it's embeded in TiDB ... just like the ddl owner

The old implementation can still be kept, its performance is better and might meet some user's needs.
We can rename the syntax for it, such as `AUTO_INCREMENT_FAST` or `AUTO_INCREMENT_OLD` or `DEFICIT_AUTO_INCREMENT`.

The new `AUTO_INCREMENT` behaviour is still a compatibility-breaker between old and new versioned TiDB, but it's worthy on behalf of MySQL compatibility.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all the auto_increment allocations done during INSERT and UPDATE done the same ways in TiDB as in MySQL? (which may be another issue of compatibility)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

INSERT and UPDATE handling is the same between TiDB and MySQL...


When the switch happen, the current max allocated ID information is required so as to guarantee the uniqueness. The new primary process allocate IDs begin from the max allocated ID plus one.

We can persist the max allocated ID every time, but that's costly. Another choice is persisting it periodically, it's safe to allocate IDs in range `[base, max persisted)`. When the primary process crash abnormally, the backup process gets the max persisted ID as its base. Etcd is also used as the storage for the max persisted ID. This optimization could still make the ID not be consecutive, but it's not so common (and I believe MySQL may also crash and facing such problem?), so this is not a big issue.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For MySQL it depends on version, it changed in 8.0 vs 5.7, see https://dev.mysql.com/doc/refman/8.0/en/innodb-auto-increment-handling.html#innodb-auto-increment-initialization

In 5.7 on (re)start of the server, it would initialise the auto_increment like SELECT MAX(auto_inc_col) FROM t, while in 8.0 it persist each allocation in the redo log. So there should not be any 'lost' values due to caching and restart.

Also there are a lot of different details that can still differ between TiDB and MySQL/InnoDB, like the setting of sysvar_innodb_autoinc_lock_mode.

But getting the basics more compatible is good!

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 19, 2022
@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2022
@tiancaiamao
Copy link
Contributor Author

/run-build-arm64 comment=true

@sre-bot
Copy link
Contributor

sre-bot commented Oct 31, 2022

@tiancaiamao
Copy link
Contributor Author

/run-build-arm64 comment=true

@sre-bot
Copy link
Contributor

sre-bot commented Oct 31, 2022

@tiancaiamao
Copy link
Contributor Author

/run-build-arm64 comment=true

@sre-bot
Copy link
Contributor

sre-bot commented Nov 1, 2022

@ti-chi-bot ti-chi-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 1, 2022
@tiancaiamao
Copy link
Contributor Author

/run-build-arm64 comment=true

@sre-bot
Copy link
Contributor

sre-bot commented Nov 2, 2022

@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 3, 2022
@ti-chi-bot
Copy link
Member

@tiancaiamao: PR needs rebase.

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.

@tiancaiamao
Copy link
Contributor Author

/run-build-arm64 comment=true

@sre-bot
Copy link
Contributor

sre-bot commented Nov 3, 2022

@tiancaiamao
Copy link
Contributor Author

This one had been splited into several PRs and merged.

@tiancaiamao tiancaiamao deleted the autoid branch June 16, 2023 16:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MySQL compatible AUTO_INCREMENT
5 participants