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

expression: fix uuid generate duplicate value with multi-node #10590

Merged
merged 4 commits into from
Jun 4, 2019
Merged

expression: fix uuid generate duplicate value with multi-node #10590

merged 4 commits into from
Jun 4, 2019

Conversation

lysu
Copy link
Contributor

@lysu lysu commented May 24, 2019

What problem does this PR solve?

In master select uuid() will got this result:

mysql> select uuid();
+--------------------------------------+
| uuid()                               |
+--------------------------------------+
| ad044d85-7dee-11e9-8000-000000000000 |
+--------------------------------------+
1 row in set (0.00 sec)

the question is 000000000000 part, it should node an IEEE 802 node number for RFC 4122 version-1

but has be 000000000000, the root question is https://github.com/myesui/uuid/blob/master/generator.go#L21 init a generator but doesn't call https://github.com/myesui/uuid/blob/master/generator.go#L230 to init node value from https://github.com/myesui/uuid/blob/master/generator.go#L508

as result, mutiple node generate uuid maybe duplicate if they be called as same time.

What is changed and how it works?

the easiest way to fix this question is call NewGenerator upper case method to init, instead of use uuid.NewV1, but after some look NewGenerator doesn't handle the question interface can not obtain, after thinking maybe we should change to https://github.com/google/uuid which 's impl is more like RFC's requirement https://github.com/google/uuid/blob/master/node.go#L39

ps: mysql's implement https://github.com/mysql/mysql-server/blob/6b10960e9d2e606b2774143da60903d641ac542f/sql/item_strfunc.cc#L3898 also will use random nodeid when interface can not obtain too, https://dev.mysql.com/doc/refman/8.0/en/miscellaneous-functions.html#function_uuid, and uuid can be duplicate but it should be very low probability

at last, our ddl use uuid.NewV4 don't have this problem will keep use old lib.

Check List

Tests

  • Unit test
  • Integration test
  • Manual test

after this patch will got

+--------------------------------------+
| uuid()                               |
+--------------------------------------+
| 08fbb5d0-7df1-11e9-80de-9cb6dx9073cb |
+--------------------------------------+

Code changes

  • impl change
  • add go.mod

Side effects

  • N/A

"Breaking backward compatibility" should not be problem, V1 use timestamp and new version will add real not it should not duplicate to old generated data by old tidb.

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

This change is Reviewable

@lysu lysu added component/expression type/bugfix This PR fixes a bug. labels May 24, 2019
@codecov
Copy link

codecov bot commented May 24, 2019

Codecov Report

Merging #10590 into master will decrease coverage by 0.0192%.
The diff coverage is 66.6666%.

@@               Coverage Diff                @@
##             master     #10590        +/-   ##
================================================
- Coverage   78.3658%   78.3465%   -0.0193%     
================================================
  Files           414        414                
  Lines         87704      87709         +5     
================================================
- Hits          68730      68717        -13     
- Misses        13840      13852        +12     
- Partials       5134       5140         +6

@lysu
Copy link
Contributor Author

lysu commented May 24, 2019

/run-all-tests

Copy link
Contributor

@alivxxx alivxxx left a comment

Choose a reason for hiding this comment

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

LGTM

@alivxxx alivxxx added the status/LGT1 Indicates that a PR has LGTM 1. label May 27, 2019
@zz-jason
Copy link
Member

after using google uuid, we can not generate the same uuid even in different tidb-servers?

@tiancaiamao
Copy link
Contributor

If multiple tidb instances are deployed on one machine, will they generate the same uuid?

@tiancaiamao
Copy link
Contributor

LGTM
Anyway, it's much better than before

@tiancaiamao tiancaiamao added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels May 28, 2019
Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/expression status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants