FreeOpcUa / python-opcua

LGPL Pure Python OPC-UA Client and Server
http://freeopcua.github.io/
GNU Lesser General Public License v3.0
1.35k stars 659 forks source link

ContinuationPoints, ServerTimestamps and System clock offsets. #561

Open brubbel opened 6 years ago

brubbel commented 6 years ago

(low priority issue) Background: I have a node with an (intentionally) messed up historian database: Server timestamps are not monotonically increasing any more. This is to simulate a system clock adjustment by the NTP server, or any other issues with the reference clock.

Problem: The current implementation of continuation points employs the last ServerTimestamp-element in the list of historical data (which is not equal to the earliest/latest time in this messed-up ordered-by-sql-id list). As a result, using this continuation point in my python-opcua client results in an indefinite loop, while UA-Expert just stops loading data beyond the affected historical data. In the python-opcua client, I can also stop loading by breaking out the loop when a duplicate continuation point comes by.

Expected: Besides the issue of wrong timestamps, I think continuation points should not be affected by system time issues so that further use of the database becomes difficult without adjustments/checks on client side.

Reference to code: https://github.com/FreeOpcUa/python-opcua/blob/bd97029821973e07c06f673753a774da270e6790/opcua/server/history_sql.py#L128-L130

oroulet commented 6 years ago

Sounds like fuzzy testing. Do you make a pr?

zerox1212 commented 6 years ago

How do you suggest we solve it? I don't know how you want to handle the server UTC time suddenly going backwards. Either you go by the database row index, but then time series data will be screwed up. Or you sort by timestamp before setting the continuation point, but then your data points will be out of order again.

I'm open to ideas because handling time in the programming world is always a big pain.

brubbel commented 6 years ago

The last few days I've been testing with the PRIMARY KEY as continuation point. Under normal conditions, sqlite sort of guarantees that this key is monotonically increasing if you don't mess with the auto-increment tables.

For example:

SELECT * FROM "2_1609830363" WHERE "_Id" <= 6229 AND "ServerTimestamp" BETWEEN ? AND ? ORDER BY "_Id" DESC LIMIT ?
('1601-01-01 00:00:00', '2018-03-02 19:55:27.763350', 897)

Where _Id=6229 is the continuation point, <= (for DESC) or >= (for ASC) depends on which direction you are reading. So, this way it will never pull the same data twice.

Of course, this does not solve the issue of incorrect timestamps, but I consider this as a separate issue. If I'm correct, there is no diversion from the old behaviour, because ORDER BY "_Id" was already there. Anyway, it seems to work and I will do a PR later on.

zerox1212 commented 6 years ago

I guess it will depend also on the reader of the data. I don't know if the history charts in UA Expert are sorting the data by index or by timestamp. I suppose getting the continuation point from the key is probably better, but I would like to see your code. I thought the OPC UA spec says a continuation point is a datetime.

oroulet commented 6 years ago

As far as remember @zerox1212 is right. But i also think it was allowed to use something else. Someone need to check spec.

oroulet commented 6 years ago

Btw i suppose the dict based implementation has same issue? Did you test?

brubbel commented 6 years ago

Small relevant extract from the OPC UA Specification Part 11: Historical Access, page 21, point 6.3:

The continuationPoint [...skip...] mark a point from which to continue the read if not all values could be returned in one response. The value is opaque for the Client and is only used to maintain the state information for the Server to continue from. For HistoricalDataNode requests, a Server may use the timestamp of the last returned data item if the timestamp is unique.

Basically, the client gets a black-box binary continuationpoint, and throws this blob back to the server when it wants to continue. https://github.com/FreeOpcUa/python-opcua/blob/bd97029821973e07c06f673753a774da270e6790/opcua/server/history.py#L315-L316 and https://github.com/FreeOpcUa/python-opcua/blob/bd97029821973e07c06f673753a774da270e6790/opcua/server/history.py#L323-L328 PS: I didn't test with the dict based version.

zerox1212 commented 6 years ago

If that's the case then it's better to just use the index of the record for continuation. I think the only reason time was used is because the limits are in datetime.