timestamptz is wrongly saved

Posts   
 
    
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 26-Oct-2017 19:01:06   

Hi guys,

LLBLGenPro 5.3 I have a postgresql's timestamptz database field (database first development). This is mapped to entity as DateTime. I would prefer DateTimeOffset but can live with it. The problem arises when saving entity. The datetime just looses timezone info (timezone isn't stored in postgres anyway) - it should be converted to UTC and then sent to the database according to Npgslq docs at http://www.npgsql.org/doc/types/datetime.html. I assume LLBLGenPro is using Npgsql's timestamp type in this case (or no type which probably defaults to timestamp).

To repro: create a postgresql timestamptz field, set entity.field = DateTime.Now and reload the entity. If timezone of the computer isn't T+0 then the saved and loaded date won't be equal.

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 26-Oct-2017 21:04:58   

We use the DateTime value, not an npgsql type. We simply send the DateTime as-is to the DB, so we don't do any conversion.

You can add this tho: use the DateTime to DateTimeUTC type converter which is shipped with llblgen pro: it will convert the value to UTC before saving and convert to the local timezone when loading (and assumes the value is in UTC coming from the DB).

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 26-Oct-2017 21:26:10   

We use the DateTime value, not an npgsql type. We simply send the DateTime as-is to the DB, so we don't do any conversion.

Yep, I thought that was the case.

You can add this tho: use the DateTime to DateTimeUTC type converter which is shipped with llblgen pro: it will convert the value to UTC before saving and convert to the local timezone when loading (and assumes the value is in UTC coming from the DB).

This will probably solve the issue. Out of curiosity, one can't set driver's data type explicitly? Perhaps something to add in the future for solving such issues?

mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 08:59:22   

DateTimeUTC type converter works.

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 09:25:02   

mihies wrote:

We use the DateTime value, not an npgsql type. We simply send the DateTime as-is to the DB, so we don't do any conversion.

Yep, I thought that was the case.

You can add this tho: use the DateTime to DateTimeUTC type converter which is shipped with llblgen pro: it will convert the value to UTC before saving and convert to the local timezone when loading (and assumes the value is in UTC coming from the DB).

This will probably solve the issue. Out of curiosity, one can't set driver's data type explicitly? Perhaps something to add in the future for solving such issues?

We do set the driver type explicitly, but the value is a datetime value. And as npgsql doesn't do any conversion, the value is passed to the db by npgsql as-is, as far as I know. So we do set the type on the parameter with the npgsql enum value, set the value (the datetime) and npgsql has to do the rest.

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 10:21:18   

Actually driver does some conversion. If you use NpgsqlDbType.Timestamp then there is no conversion whatsoever (time zone is simply ignored and date is stored as is) while if you use NpgsqlDbType.TimestampTZ then driver converts local to utc date and then it stores it. See http://www.npgsql.org/doc/types/datetime.html

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 10:47:42   

mihies wrote:

Actually driver does some conversion. If you use NpgsqlDbType.Timestamp then there is no conversion whatsoever (time zone is simply ignored and date is stored as is) while if you use NpgsqlDbType.TimestampTZ then driver converts local to utc date and then it stores it. See http://www.npgsql.org/doc/types/datetime.html

but you said in your first post:

The datetime just looses timezone info (timezone isn't stored in postgres anyway) - it should be converted to UTC and then sent to the database according to Npgslq docs at http://www.npgsql.org/doc/types/datetime.html.

which contradicts that statement wink (or at least makes me confused). Anyway -> I see we map it in the driver to the provider type 'Timestamp', not 'TimestampTZ'. This might be a leftover of npgsql v2 tho, I have to check.

Btw, shouldn't you use Timetz ? That's mapped to a DateTimeOffset

Frans Bouma | Lead developer LLBLGen Pro
Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 11:03:27   

Could you test the attached v5.3 driver for postgresql? It maps Timestamptz now to the provider type timestamptz

You should be able to just re-generate the code and it should work (remove the type converter again!)

Attachments
Filename File size Added on Approval
SD.LLBLGen.Pro.DBDrivers.PostgreSqlDBDriver.dll 69,632 27-Oct-2017 11:03.47 Approved
Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 11:09:53   

"but you said in your first post: ..."

AFAIK postgresql doesn't store timezone. Npgsql.[type]tz just instructs client (or more probably server, don't know) to convert from local to utc before the value is stored in database while Npgsql.[type] without tz stores local datetime as it appears.

Indeed it is a bit confusing combined with my post simple_smile

Anyway, will try the new driver.

mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 11:30:58   

For some reason I'm getting this error now (renamed original driver to dll.original and copy&pasted new one)

Exception message:

Exception type: XmlException The XML contains a reference to a driver with ID: '88EBFD8C-CBDD-4452-88AF-1C99E41A123F' which isn't loaded, likely due to a missing ADO.NET provider. Line 240, Position 6

Stack trace LLBLGen Pro version 5.3. Build 5.3.0 -----[Core exception]-------------------- at SD.LLBLGen.Pro.ApplicationCore.ProjectClasses.Project.DeserializeFromFile(String filename, String additionalTypeConverterFolder) at SD.LLBLGen.Pro.ApplicationCore.ProjectClasses.Project.Load(String filename, String additionalTypeConverterFolder) at SD.LLBLGen.Pro.Gui.Classes.GuiController.PerformOpenProjectAction(String filenameToOpen)

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 11:43:00   

You have to unblock the dll I think. Right-click the dll in windows explorer -> properties -> unblock

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 11:53:18   

Otis wrote:

You have to unblock the dll I think. Right-click the dll in windows explorer -> properties -> unblock

Argh, yes. And it has to be writable (not read-only) othwerise one gets an "assert failure" upon unblocking.

mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 12:03:32   

Otis wrote:

Could you test the attached v5.3 driver for postgresql? It maps Timestamptz now to the provider type timestamptz

You should be able to just re-generate the code and it should work (remove the type converter again!)

Still doesn't work. But I think I know why. AddElementFieldMapping is setting dbtype to "TimestampTz" while Npgsql uses NpgsqlDbType.TimestampTZ). Could this be the reason?

mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 12:22:48   

Yep, that's it.

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 12:53:46   

Hmm, I looked at the code! -> https://github.com/npgsql/npgsql/blob/dev/src/Npgsql/NpgsqlTypes/NpgsqlDbType.cs#L244

The one with TZ is deprecated. You use the latest npgsql?

However this does bode a problem... I can only map 1 type, not 2. If I pick TZ, it's obsolete. If I pick Tz, people might run into issues. sigh

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 14:12:27   

I see. I also see that the commit making TimestampTZ and introducing TimestampTz has been made after 3.2.5 was published.

I'd say go with TimestampTZ since, while (edit: not yet) obsolete, it will be around for a while. Even more so because TimestampTz isn't there yet (NuGet package I mean). And switch to TimestampTz once Npgsql NuGet package is updated. Makes sense?

mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 14:19:50   

Also 3.2.6 https://github.com/npgsql/npgsql/milestone/48 isn't around the corner, it seems (with 75% completed).

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 14:33:24   

Ok, but either way... this is very fragile. They can flip the switch on the deprecated attribute to 'error' at any moment and the code of our users then breaks without option to fix unless the driver is updated but they'll not know that.

So to fix this I have to add a mapping to a value which is deprecated in the next release and can't add a mapping to the one I should use today.

I'll leave it as-is for now and roll back the change, and the Type converter has to be used for the time being. I'll schedule the change for a later date when npgsql is updated and more is known about what to do with this.

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 14:47:37   

I can live with type converters.

My thinking: If you leave it as is, it means a lot of type converters assignments in designer and another angle of possible problems. Also I don't think they'll deprecate it anytime soon as they probably have a ton of legacy code. Even if they did llblgen would fallback to the current behavior.

mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 14:53:02   

Another thought. If we go with type converters and you eventually switch to TimeStampT[zZ] I'm not sure whether we'll get correct result unless type converters are removed.

Tricky, indeed.

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 27-Oct-2017 15:47:51   

mihies wrote:

I can live with type converters.

My thinking: If you leave it as is, it means a lot of type converters assignments in designer and another angle of possible problems. Also I don't think they'll deprecate it anytime soon as they probably have a ton of legacy code. Even if they did llblgen would fallback to the current behavior.

Hmm. I'm thinking about the situation where they remove the deprecated enum value, as it's deprecated and therefore not usable. If they remove it, we have a problem.

No it's not a problem if the typeconverter is in place and we do switch: the DateTime send to the DB in the parameter has the UTC flag set, so npgsql will see it as a UTC value, and won't convert (according to the docs).

The typeconverter usage isn't ideal, admitted... I'll ask Roji if the value will ever be removed.

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 27-Oct-2017 15:56:58   

Let's say I use an utc type converter. And you someday enable timestamptz. Would it work like: you get a local datetime from npgsql driver, you take it's UTC value and convert it to local time (basically to the same value as Npgsql driver returns)? If that's the case, then it is fine.

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 28-Oct-2017 10:15:57   

ah, you mean, the value is first converted to local time by npgsql and then the typeconverter changes it again thinking it's an UTC time? Yes that'll work, as the typeconverter uses ToLocalTime() and ToUniversalTime(), which check first whether the value is already of the kind they need to return. So a local datetime on which you call ToLocalTime() will simply return the same value. See the sourcecode of DateTime.ToLocalTime().

However I've opened an issue on npgsql (https://github.com/npgsql/npgsql/issues/1694) regarding whether they'd remove TimestampTZ or not, and Roji said it's highly unlikely but could be, however he has added a remark to the enum value referencing my concern so we're pretty safe. I'll make the change next week.

Frans Bouma | Lead developer LLBLGen Pro
mihies avatar
mihies
User
Posts: 800
Joined: 29-Jan-2006
# Posted on: 28-Oct-2017 18:51:12   

Ok, great, thanks.

Otis avatar
Otis
LLBLGen Pro Team
Posts: 39614
Joined: 17-Aug-2003
# Posted on: 30-Oct-2017 11:24:51   

Now available in hotfix builds 5.2.4 and 5.3.1

Frans Bouma | Lead developer LLBLGen Pro