openhab / openhab-addons

Add-ons for openHAB
https://www.openhab.org/
Eclipse Public License 2.0
1.87k stars 3.58k forks source link

[jdbc] Charts don't show the correct dates/times because persistence doesn't check with the right timezeone #13775

Open edman007 opened 1 year ago

edman007 commented 1 year ago

Hi, When I chart my data, it does not show the most recent data, this happened in OpenHab 2.5.x, I recently upgraded to 3.3.0, and it's still happening. Specifically, I live in New York, my timezone is America/New_York (GMT-05:00). I have this correctly set in the region settings.

Expected Behavior

Charts should show up to the second data

Current Behavior

Charts omit the last 5 hours of data. screenshot

Possible Solution

Queries should be written in UTC, I do not beleive the fix in https://github.com/openhab/openhab-addons/pull/9445 was ever actually correct

Steps to Reproduce (for Bugs)

  1. Set timezone to something other than UTC (I think it needs to be a GMT-XX:XX. America/New_York is a good example
  2. Persist some data
  3. Chart the data

Context

This is related to the bugs referenced in https://github.com/openhab/openhab-addons/pull/9445

I can see that the queries insert with NOW() for time, and that's stored in a native database format. That means dates/times need to be queried and displayed in the connection local. This typically should be UTC for a variety of reasons, regardless of the users local.

However, that's not what's happening, the insert of data looks like this (and it's correct) 11:26:29.069 [DEBUG] [g.openhab.persistence.jdbc.dto.ItemVO] - JDBC:ItemVO tableName=solar_system_power_0103; newTableName=null; 11:26:29.070 [DEBUG] [enhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::getItemType: Try to use ItemType NUMBERITEM for Item solar_system_power 11:26:29.070 [DEBUG] [enhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::storeItemValueProvider: item 'solar_system_power' as Type 'NUMBERITEM' in 'solar_system_power_0103' with state '3.926' 11:26:29.070 [DEBUG] [enhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::storeItemValueProvider: itemState: '3.926' 11:26:29.071 [DEBUG] [g.openhab.persistence.jdbc.dto.ItemVO] - JDBC:ItemVO setValueTypes dbType=DOUBLE; javaType=class java.lang.Double; 11:26:29.071 [DEBUG] [enhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::storeItemValueProvider: newVal.doubleValue: '3.926' 11:26:29.071 [DEBUG] [enhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::doStoreItemValue sql=INSERT INTO solar_system_power_0103 (TIME, VALUE) VALUES( NOW(3), ? ) ON DUPLICATE KEY UPDATE VALUE= ? value='3.926' 11:26:29.073 [DEBUG] [.jdbc.internal.JdbcPersistenceService] - JDBC: Stored item 'solar_system_power' as '3.926' in SQL database at Thu Nov 24 11:26:29 EST 2022 in 4 ms.

However, the querying of that same data looks like this ` 5001-2022-11-24 11:33:37.025 [DEBUG] [jdbc.internal.JdbcPersistenceService] - JDBC::query: item is solar_system_power 5002-2022-11-24 11:33:37.026 [DEBUG] [persistence.jdbc.internal.JdbcMapper] - JDBC::getHistItemFilterQuery filter='true' numberDecimalcount='3' table='solar_system_power_0103' item='solar_system_power (Type=NumberItem, State=4.2055, Label=Current Solar Power [%.3f kW], Category=energy, Groups=[SolarSystem])' itemName='solar_system_power' 5003-2022-11-24 11:33:37.026 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::getHistItemFilterQueryProvider filter = FilterCriteria [itemName=solar_system_power, beginDate=2022-11-23T16:33:37.014Z, endDate=2022-11-24T16:33:37.013Z, pageNumber=0, pageSize=2147483647, operator=EQ, ordering=ASCENDING, state=null], numberDecimalcount = 3, table = solar_system_power_0103, simpleName = solar_system_power 5004:2022-11-24 11:33:37.027 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::query queryString = SELECT time, value FROM solar_system_power_0103 WHERE TIME>'2022-11-23 11:33:37' AND TIME<'2022-11-24 11:33:37' ORDER BY time ASC 5005:2022-11-24 11:33:37.028 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::doGetHistItemFilterQuery sql=SELECT time, value FROM solar_system_power_0103 WHERE TIME>'2022-11-23 11:33:37' AND TIME<'2022-11-24 11:33:37' ORDER BY time ASC 5006-2022-11-24 11:33:37.029 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::ItemResultHandler::handleResult getState value = '0.0084', unit = 'null', getClass = 'class java.lang.Double', clazz = 'Double' 5007-2022-11-24 11:33:37.030 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::ItemResultHandler::handleResult getState value = '0.008', unit = 'null', getClass = 'class java.lang.Double', clazz = 'Double' 5008-2022-11-24 11:33:37.030 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::ItemResultHandler::handleResult getState value = '0.0081', unit = 'null', getClass = 'class java.lang.Double', clazz = 'Double'

5097-2022-11-24 11:33:37.074 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::ItemResultHandler::handleResult getState value = '0.0', unit = 'null', getClass = 'class java.lang.Double', clazz = 'Double' 5098-2022-11-24 11:33:37.074 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::ItemResultHandler::handleResult getState value = '0.0069', unit = 'null', getClass = 'class java.lang.Double', clazz = 'Double' 5099-2022-11-24 11:33:37.074 [DEBUG] [nhab.persistence.jdbc.db.JdbcBaseDAO] - JDBC::ItemResultHandler::handleResult getState value = '0.0', unit = 'null', getClass = 'class java.lang.Double', clazz = 'Double' 5100-2022-11-24 11:33:37.075 [DEBUG] [jdbc.internal.JdbcPersistenceService] - JDBC: Query for item 'solar_system_power' returned 94 rows in 49 ms ` The previous pull request seems to have changed it to query in localtime which isn't correct, it should query in UTC and then convert the results to localtime Querying in localtime? ` //JdbcBaseDAO.java filterString += " TIME>='" + JDBC_DATE_FORMAT.format(filter.getBeginDate().withZoneSameInstant(timeZone)) ` ## Your Environment debian bullseye openhab 3.3.0
jlaur commented 1 year ago

@edman007 - two initial questions:

  1. Which database are you using?
  2. What is the value of your url configuration parameter? See https://www.openhab.org/addons/persistence/jdbc/#configuration
edman007 commented 1 year ago
  1. mariadb
  2. Right now: jdbc:mariadb://localhost/openhab?useUnicode=true&useJDBCCompliantTimezoneShift=true&useLegacyDatetimeCode=false&serverTimezone=UTC

I have tried setting serverTimeZone, connectionTimeZone, useLegacyDatetimeCode (all combinations of the three, trying UTC and America/New_York, none of it matters), switching to America/New_York in the url shifts the times by 5 hours but doesn't cause current data to be fetched

jlaur commented 1 year ago

@edman007 - which version of MariaDB? I'm not sure those options exist anymore: https://mariadb.com/kb/en/about-mariadb-connector-j/#timezone-consideration

You could try: jdbc:mariadb://localhost/openhab?useUnicode=true&timezone=America/New_York

Also, for a table having the issue, what is the type of the time column? And are timestamps persisted correctly (i.e. looks correct when using some other client to query data)?

I can add that I'm using MySQL without any issues and I'm 1 or 2 hours from UTC (depending on DST), so that would also have been an issue for me if it didn't work. I have url=jdbc:mysql://fitty:3306/openhab?serverTimezone=Europe/Copenhagen

jlaur commented 1 year ago

@edman007 - btw, the MariaDB driver was upgraded recently: #13659. You could also try this version: org.openhab.persistence.jdbc-3.4.0-SNAPSHOT.jar with client 3.0.8 - see #13765. This way you would have the latest six years of changes in the driver, which you are currently missing in the 3.3 version.

edman007 commented 1 year ago

So I think I poked around down the connector bug route, and I got it working

jdbc:mariadb://localhost/openhab?useUnicode=true&serverTimezone=America/New_York&sessionVariables=time_zone='America/New_York'

Which comes from a few things, number 1 there is a connector bug: https://jira.mariadb.org/browse/CONJ-433 severTimeZone as you pointed out doesn't work, also timezone doesn't work (I tried that too), but forcing the session variable DOES correctly make the connection use the right timezone, and then you still have to set serverTimezone so the connector knows it too.

That said, I think this is still a bug in the jdbc persistence, since queries always need to be in the connection timezone, and the connection should be in UTC (because the DST overlap makes ambiguous dates and it becomes impossible to actually query those time correctly). What I have now works, but only because I forced jdbc to use the same timezone as the openhab (which shouldn't be a requirement).

Also, I think part of the problem is the effect that I'm seeing is that the window that charts query is offset by the GMT offset, for those in Europe, with a small positive offset, the effect would be that when configured to so the past 24 hours, you'd get just the past 23 hours which is not all that noticeable. But those with a negative GMT offset lose access to live data entirely.

Anyways, I will try the new connector see if that changes anything.

jlaur commented 1 year ago

@edman007 - I will also try to have another look, but can't promise anything. It's a complex topic, many factors are involved. Just from the top of my head:

I don't know details about all supported databases, but can imagine quirks and differences. Just as an example, in Microsoft SQL Server, the type DATETIME doesn't store or consider any time zone. So you can store UTC, local time or whatever you want, but you have to know what you stored and handle any conversions yourself - for example when mapping it to C# struct DateTime which can be either local or UTC.

In MySQL (and probably the same in MariaDB) at least there is some support for time-zones and conversions:

"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."

Until today with #13765 some of my (old) tables had DATETIME columns for time and others had TIMESTAMP(3). Without the proper amount of controls/configuration or simply with some wrong assumptions, this could have been a disaster.

Notes:

jlaur commented 1 year ago

That said, I think this is still a bug in the jdbc persistence, since queries always need to be in the connection timezone, and the connection should be in UTC (because the DST overlap makes ambiguous dates and it becomes impossible to actually query those time correctly). What I have now works, but only because I forced jdbc to use the same timezone as the openhab (which shouldn't be a requirement).

You have valid point. I think session time-zone should be set to UTC by the JDBC persistence service, so that queries could also be generated with UTC times, not local times. For example, in MySQL: SET time_zone = '+00:00'

edman007 commented 1 year ago

@jlaur I think you don't want to use SET time_zone, the issue is jdbc parses the timezone from the connection parameters itself (so when you do serverTimezone=XYZ, the connector configures the database<->java timezone automatic conversion itself, based on the timezone that the connection is configured as). Doing a SET bypassed this detection and causes timezone errors. That's why my "workaround" works, you have to set the serverTimezone which configures the connector timezone conversion, you have to set the time_zone variable which configures the connection, and they BOTH have to match the openhab timezone because the persistance is doing it's own conversion as well. So doing a SET time_zone query in the persistence will just screw up the conversion on the response.

That leaves you with two options that will work and have the intended results:

  1. The filter in the openhab persistence should NOT perform any timezone conversion, this needs to be done by not putting the date in the query directly, you instead use a prepared statement to write the filter query. The connector will then handle the timezone conversion based on what it understands to be the timezone of the connection
  2. Openhab persistence can query the connection timezone, then use that timezone when creating timestamp strings in queries.

Both of these should make the implementation immune to the actual timezone selected by the user (and should help reduce problems upgrading to the change, because it will still work for all the people that put their local timezone in the jdbc url). Then you just put a doc note that UTC is recommended for the jdbc timezone regardless of the openhab timezone.

Also, if another database is sensitive to the connection timezone (and people can't change their database to UTC because the database can't do timezone conversions, then this keeps the effect of the timezone in the jdbc url)

jlaur commented 1 year ago

@edman007 - all you wrote seems sensible. I have made some tests, but before migrating queries to prepared statements, I wanted to check the behavior of time-zone conversions for timestamps in result sets. This: https://github.com/openhab/openhab-addons/blob/45a3054c76200752f483d3e8433d327451c4a304/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcBaseDAO.java#L715

gives me some headache. java.sql.Timestamp is derived from the old Date class and should reflect UTC. However, when converting to Instant, it seems that connection time-zone is considered (in this example I'm using serverTimezone=America/New_York, openHAB system time-zone is Europe/Copenhagen (which is UTC+1 w/o DST) and queried timestamp is 2022-12-04T21:02:24.164Z):

So it seems the actually received java.sql.Timestamp is converted already according to connection time-zone. I have no idea how to prevent that. And it probably wouldn't help to accommodate by applying the connection time-zone offset, since we could already have ambiguous times because of DST. The only real solution would be to keep everything in UTC on the database side.

This is the query made before calling objectAsZonedDateTime: https://github.com/openhab/openhab-addons/blob/45a3054c76200752f483d3e8433d327451c4a304/bundles/org.openhab.persistence.jdbc/src/main/java/org/openhab/persistence/jdbc/internal/db/JdbcBaseDAO.java#L465-L480

edman007 commented 1 year ago

@jlaur So I too tried doing the conversions and I basically stumbled on the same thing, java.sql.timestamp is having a timestamp conversion being applied incorrectly.

I'm going to try and dig through the source, as deep as I need to, and find where that bug is, I believe it's a mariadb connector bug, but I need to trace it down (I need to rule out knowm Yank first I think).

Right now I'm leading to the fact that the mariadb connector does NOT handle timezone conversions at all, and does NOT have a way to detect, via the connector, what timezone there is, and some very bad hacks are likely required. This is very likely a mariadb connector bug (which I find very surprising, since it's a mature product).

If I find a good solution I'll be sure to post it, though right now, all I have is I can say:

  1. connectionTimezone=XYZ/serverTimezone=XYZ does NOT work in openhab 3.2.x because of connector bugs, my workaround is needed for it to work predictably
  2. timezone=XYZ DOES work in openhab 3.3.0, this is an API change for the JDBC URL, might want to include that in the release notes
  3. Openhab currently requires that when using mariadb, timezone=XYZ MUST be set to the openhab local, it does NOT work any other way.
  4. objectAsZonedDateTime() uses ZoneId.SystemDefault(), this is a bug, doGetHistItemFilterQuery() is called with a zoneId, that needs to be passed to objectAsZonedDateTime(), I'm not sure how much this matters, I haven't tested it, that may always be the same, but I suspect not. The intention is the timezone in doGetHistItemFilterQuery() is for objectAsZonedDateTime() ONLY. The filter is passed a ZonedDateTime for the start/end, that should be converted to the connection timezone (though the responsibility is probably up in the air as a result of this bug)
  5. Somewhat unrelated, but f9aa20e488adfeeb9183dbd65ce78dd9037abbc7 which fixed #9906 is incomplete, they missed JdbcPostgresqlDAO.java and JdbcDerbyDAO.java
jlaur commented 1 year ago
  • objectAsZonedDateTime() uses ZoneId.SystemDefault(), this is a bug, doGetHistItemFilterQuery() is called with a zoneId, that needs to be passed to objectAsZonedDateTime(), I'm not sure how much this matters, I haven't tested it, that may always be the same, but I suspect not.

I found that also and have a local fix for it using TimeZoneProvider. It can be different when the openHAB server is hosted in another time-zone.

I missed this, thanks for finding that. I will create a fix.

edman007 commented 1 year ago

@jlaur I think for that filterquery fix, it's JdbcDerbyDAO.java too, not just Postgres

Anyways, I did more testing on this issue, looks like it is the connector, I verified that the mysql connector does NOT have the same bug, found an existing bug ( https://jira.mariadb.org/browse/CONJ-964 ) and gave them a proper test case, see if anything comes of that, and yea, it's hard to fix the timezones in openhab when the connector can't get it right...maybe fix the timezone stuff and tell people on mariadb to use the mysql connector if they have timezone issues, not sure if that's really a good solution though

jlaur commented 1 year ago

I think for that filterquery fix, it's JdbcDerbyDAO.java too, not just Postgres

Oh, this is really starting to look bad. :blush: Thanks gain for catching that. I even did a text search yesterday, and still I somehow missed it. I will fix that too in the evening (CET).

Anyways, I did more testing on this issue, looks like it is the connector, I verified that the mysql connector does NOT have the same bug

So it must be somewhere else then? My tests yesterday were with MySQL Connector.

openhab-bot commented 1 year ago

This issue has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/openhab-3-4-release-discussion/142257/118

lsiepel commented 2 months ago

As multiple PR's have been merged that also seem related to the timezone, connector and/or timestamp i wonder if there is anything more to do here? Please confirm with openHAB 4.2.0 and reproducable configuration/setup if this is not fixed yet.

XypersonicOne commented 1 month ago

I think there could still be a problem related to timezone/jdbc/charts... Im missing 2 Hours of data in oh-time-series using openHAB 4.2.1.

In my Configuration i am using a MySQL database via JDBC to store the values. The database stores the values in UTC time, and the values for the period from 0:00 to 02:00 are also present. Openhab and OpenhabServer run on TimeZone=Europe/Vienna. For JDBC, I specify the following: url=jdbc:mysql://home:3306/oh?useUnicode=true&useJDBCCompliantTimezoneShift=true&useLegacyDatetimeCode=false&serverTimezone=UTC.

I postet this already to oh-community. https://community.openhab.org/t/missing-values-of-2-hours-in-oh-time-series-day-chart-timezoneproblem/157613

In fact the 2 Hours of Data at the beginning are Missing at other Periods like 4h too... Bildschirmfoto 2024-08-16 um 11 05 59