-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Change in GET_LOCK call to make it MariaDB compatilble #5343
Change in GET_LOCK call to make it MariaDB compatilble #5343
Conversation
Change in GET_LOCK call to make it MariaDB compatilble
physical/mysql/mysql.go
Outdated
@@ -620,7 +620,7 @@ func (i *MySQLLock) becomeLeader() error { | |||
func (i *MySQLLock) Lock() error { | |||
defer metrics.MeasureSince([]string{"mysql", "get_lock"}, time.Now()) | |||
|
|||
rows, err := i.in.Query("SELECT GET_LOCK(?, -1), IS_USED_LOCK(?)", i.key, i.key) | |||
rows, err := i.in.Query("SELECT GET_LOCK(?, 4294967295), IS_USED_LOCK(?)", i.key, i.key) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How was this number chosen?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This number is the decimal representation of maximal integer with 64bit length. In hexadecimal this number should be 0xffffffff
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we instead use math.MaxUint32
from the https://golang.org/pkg/math/ package and add a comment above that about -1
not being compatible with different MySQL flavours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@michaeljs1990 Yes, we can use this constant. I tested this, committed changes and comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem faced. Can you help with steps on how to apply this fix on a running mysql deployment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You shouldn't need to make any changes to mysql for this fix. What is the issue you are running into?
@michaeljs1990 could you take a look at this? |
Thanks for the improvement to make this work on MariaDB! I admit the -1 is a rather undocumented get lock feature but it seemed common enough in other software I had worked with I didn't think it would cause issues. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
Change in GET_LOCK call to make it MariaDB compatilble. This should resolve Issue 5266
The fix creates a lock with a timeout of 136 years which is failry enough to be considered as "unlimited"