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

Use DATETIME instead of TIMESTAMP for MySQL schema #514

Open
xak2000 opened this issue Jul 12, 2024 · 0 comments
Open

Use DATETIME instead of TIMESTAMP for MySQL schema #514

xak2000 opened this issue Jul 12, 2024 · 0 comments

Comments

@xak2000
Copy link

xak2000 commented Jul 12, 2024

Expected Behavior

All timestamp columns should have type DATETIME for MySQL reference definition.

Current Behavior

Currently in the MySQL reference definition all timestamp columns have TIMESTAMP type.

Related comment where @kagkarlsson explains why TIMESTAMP type was selected instead of DATETIME.

I'd like to explain why this is the wrong type for the MySQL definition in my opinion and why DATETIME should still be preferred.

I agree with the statement in the mentioned comment that TIMESTAMP is more directly mappable to java.time.Instant while DATETIME is a sort of java.time.LocalDateTime. Both do not contain a timezone, though.

But TIMESTAMP in MySQL is not direct replacement for TIMESTAMP in Posgresql. Unlike Posgresql, in MySQL TIMESTAMP has many restrictions and quirks.

For instance:

  • TIMESTAMP has a range of 1970-01-01 00:00:01 UTC to 2038-01-19 03:14:07 UTC. Probably it is not the biggest problem per se, but it is still a problem. For instance, Schedule.NEVER used by this library sets timestamp to 2500-01-01T12:00:00.00Z, that is already out of the range for MySQL TIMESTAMP data type. I didn't test what exactly will happen when the library will try to set the DB column to this value, but trying to set it manually by SQL query returns this error:

    SQL Error [1292] [22001]: Data truncation: Incorrect datetime value: '2500-01-01 12:00:00.00' for column 'execution_time' at row 1
    
  • MySQL implicitly adds DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6) to the first TIMESTAMP column in the table. A quote from the documentation:

    TIMESTAMP and DATETIME columns have no automatic properties unless they are specified explicitly, with this exception: If the explicit_defaults_for_timestamp system variable is disabled, the first TIMESTAMP column has both DEFAULT CURRENT_TIMESTAMP and ON UPDATE CURRENT_TIMESTAMP if neither is specified explicitly.

    In the scheduled_tasks table the first timestamp column is execution_time, so, despite the fact that the definition is just execution_time timestamp(6) NOT NULL, the actual definition after table is created is execution_time timestamp(6) NOT NULL DEFAULT CURRENT_TIMESTAMP(6) ON UPDATE CURRENT_TIMESTAMP(6)!

    This definition makes the execution_time column to be updated to the CURRENT_TIMESTAMP every time when any update is performed, that doesn't include execution_time column in the list of updated columns. For example, when picked column is updated after task is started. I'm not sure how many problems this could cause for the library logic, but I'm pretty sure this is not the behaviour intended/expected by the library. :)

    Note: If I understand correctly, this behaviour is disabled by default starting from MySQL 8.x. So, not all MySQL users are probably affected.

Based on my practice, the most used MySQL data type for timestamps is DATETIME. The TIMESTAMP type is too strange and quirky and usually avoided by people who uses MySQL. (unlike Postgresql's TIMESTAMP, which is totally fine).

About the fact that DATETIME doesn't have a timezone. It is true, so the best practice is to convert any timestamps into UTC timezone before storing them into DATETIME column. I think the alwaysPersistTimestampInUtc flag should cover this, right?

Btw, I think MySQL JDBC driver actually manipulates timezones based on the server time zone and the JVM timezone (or passed Calendar instance) for both DATETIME and TIMESTAMP column types. The difference is just the internal representation of the data: TIMESTAMP is storead as a number (since epoch) on the mysql server (4 bytes), while DATETIME is stored in a more complex data format (8 bytes). But TIMESTAMPs timezone is still manipulated when it is read/written. So, it is not better than DATETIME for mapping java.time.Instant. In both cases JDBC driver first formats the Instant into string in the default (or specified) JVM timezone and then into Mysql session's timezone. After that DATETIME is stored as is, while TIMESTAMP is additionally converted from the session's timezone to UTC.

A quote from the docs:

MySQL converts TIMESTAMP values from the current time zone to UTC for storage, and back from UTC to the current time zone for retrieval. (This does not occur for other types such as DATETIME.) By default, the current time zone for each connection is the server's time. The time zone can be set on a per-connection basis. As long as the time zone setting remains constant, you get back the same value you store. If you store a TIMESTAMP value, and then change the time zone and retrieve the value, the retrieved value is different from the value you stored. This occurs because the same time zone was not used for conversion in both directions. The current time zone is available as the value of the time_zone system variable. For more information, see Section 5.1.13, “MySQL Server Time Zone Support”.

P.S. I'm a new user of this library so I didn't intensively tested it with DATETIME columns yet. But I already see these problems with TIMESTAMP column type. I didn't noticed any problems when changed type of all TIMESTAMP columns to DATETIME in scheduled_tasks table, but I should mention that both my MySQL server and JVM are configured to have UTC timezone, so I could probably miss some problems related to timezone conversions done by MySQL JDBC driver. So, a more throuthful testing is required to make sure DATETIME columns will always work as expected.

But, based on what I see in DefaultJdbcCustomization the way Instant is get/set when persistTimestampInUTC is enabled is exactly the same as I usually do in all projects that work with MySQL DATETIME columns. So, I don't see why DATETIME instead of TIMESTAMP may not work as expected. And when persistTimestampInUTC is disabled, then the problem with wrong stored timestamps (when the session timezone is changed) arises for both DATETIME and TIMESTAMP column types. That is why the flag was introduced in the first place, if I understand correctly. :)

Context

  • DB-Scheduler Version : 14.0.2
  • Java Version : 21
  • Spring Boot (check for Yes) : [x]
  • Database and Version : MySQL 5.7.40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant