-
Notifications
You must be signed in to change notification settings - Fork 2k
Fix: MySQL/MariaDB TIMESTAMP precision for message ordering (#3442) #3443
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
Conversation
…rojects#3442) Fixes spring-projects#3442 Signed-off-by: Seungwon Lee <aahhll654@gmail.com>
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.
I think this PR itself is fine, but as @markpollack previously mentioned:
this field is actually being used as an auto-incrementing sequence to ensure message ordering, rather than representing time in a temporal sense. Perhaps we should consider renaming this field in the future.
|
There is a common belief that https://stackoverflow.com/questions/409286/should-i-use-the-datetime-or-timestamp-data-type-in-mysql |
|
yea, I'm not sure what to do here in a few ways.
As for point 2, a migration script can be provided to run to upgrade to 1.0.1 and advertised in the release notes etc. I'm thinking for the 1.0.x line, we provide the migration script. thoughts @YunKuiLu @sunyuhan1998 @seungwone @ThomasVitale ? |
Maybe we can do something like this:
These are just my initial thoughts though — not sure if we really need this many steps. |
|
I think I found an easier way, since the values of the timestamps are meaningless, they just need to increase, we make sure that when we are assigning the timestamp, it is always increases at an appropriate level of granulatirty - eg. one second or something like that. |
|
@seungwone Thanks for working on this! After reviewing the discussion and based on Mark's suggestion about fixing the granularity in code, here is a more fine-grained approach that doesn't require any schema breaking changes in Plan:
This approach avoids migration complexity for existing users while solving the ordering issue across all databases. Here is the new PR: #4739 Also closing this PR in preference to the new one. I created an issue for tracking all the breaking changes we discussed above (dropping the timestamp column altogether and using a sequence_id column) for the 2.0.x timeline. |
Fixes #3442
The
AddBatchPreparedStatementclass usesAtomicLong.getAndIncrement()to create sequential millisecond timestamps, but MySQL/MariaDB's default TIMESTAMP precision (0) truncates this information, causing message ordering issues.Added fractional seconds precision (TIMESTAMP(3)) to MySQL and MariaDB schemas to support millisecond-based message ordering in
JdbcChatMemoryRepository.Changes
schema-mysql.sql: ChangedTIMESTAMPtoTIMESTAMP(3)schema-mariadb.sql: ChangedTIMESTAMPtoTIMESTAMP(3)