TimestampTZ column returns Datetime not DatetimeOffset #11

Closed
schotime opened this Issue Jan 21, 2013 · 31 comments

7 participants

@schotime

Hi,

I'm trying the following:

using (var conn = new Npgsql.NpgsqlConnection("Server=127.0.0.1;Database=test;User Id=;Password=;"))
using (var com = new NpgsqlCommand("select lastmodfied from passwords", conn))
{
    conn.Open();
    var reader = com.ExecuteReader();

    while (reader.Read())
    {
        var date = (DateTimeOffset)reader.GetValue(0);
    }
}

however its returning a DateTime. How do I get a DateTime Offset from a TimestampTZ?

Cheers,
Adam

@danbarua

I don't see any follow up discussion at http://pgfoundry.org/tracker/index.php?func=detail&aid=1011335&group_id=1000140&atid=590
I'm still being bitten by this issue.

@franciscojunior
Npgsql member

Sorry. Initially we were going to use pgfoundry forums for bug reports. We are now using github. I reopened this issue so we check what is going on.

@danbarua

Thanks, I'd be happy to submit a pull-request, though not being very familiar with the Npgsql codebase I can not comment on the potential impact of fixing it for now.

@franciscojunior
Npgsql member

Did you try the GetProviderSpecificValue() method?

@danbarua

Hi,

I've played with it and almost got it working but then I came up against the following edge case:

Note that before 1940 the time zone is not just a hour offset but a hour:minute:(and prior to 1937 even second) offset.

http://lists.pgfoundry.org/pipermail/npgsql-devel/2010-January/001063.html
http://postgresql.1045698.n5.nabble.com/npgsql-Npgsql2-Added-a-test-for-Europe-Amsterdam-timezone-with-dates-td2112154.html

.Net's DateTimeOffset class will only accept an offset in whole minutes, and will fail for DateTimes in Amsterdam up to 1937.

A good enough workaround for me is to enable UseExtendedTypes and cast NpgsqlTimeStampTZ to DateTimeOffset.

@asymetrixs

I just checked the code, despite from the specials mentioned above, conversion looks to be implemented correctly:
Timestamp <-> DateTime
TimestampTZ <-> DatetimeOffset
Interval <-> TimeSpan

in NpgsqlTypesHelper.cs

nativeTypeMapping.AddType("timestamptz", NpgsqlDbType.TimestampTZ, DbType.DateTime, true,
                                ExtendedNativeToBackendTypeConverter.ToTimeStamp);

nativeTypeMapping.AddTypeAlias("timestamptz", typeof(NpgsqlTimeStampTZ));

nativeTypeMapping.AddDbTypeAlias("timestamptz", DbType.DateTimeOffset);

nativeTypeMapping.AddTypeAlias("timestamptz", typeof(DateTimeOffset));

nativeTypeMapping.AddType("abstime", NpgsqlDbType.Abstime, DbType.DateTime, true,
                                ExtendedNativeToBackendTypeConverter.ToTimeStamp);

nativeTypeMapping.AddType("timestamp", NpgsqlDbType.Timestamp, DbType.DateTime, true,
                                ExtendedNativeToBackendTypeConverter.ToTimeStamp);

nativeTypeMapping.AddTypeAlias("timestamp", typeof (DateTime));
nativeTypeMapping.AddTypeAlias("timestamp", typeof (NpgsqlTimeStamp));

in NpgsqlProviderManifest.cs

case PrimitiveTypeKind.DateTime:
    if (edmType.Facets.TryGetValue(PrecisionFacet, false, out facet) &&
        !facet.IsUnbounded && facet.Value != null)
    {
        return TypeUsage.CreateDateTimeTypeUsage(StoreTypeNameToStorePrimitiveType["timestamp"], (byte)facet.Value);
    }
    else
    {
        return TypeUsage.CreateDateTimeTypeUsage(StoreTypeNameToStorePrimitiveType["timestamp"], null);
    }
case PrimitiveTypeKind.DateTimeOffset:
    if (edmType.Facets.TryGetValue(PrecisionFacet, false, out facet) &&
        !facet.IsUnbounded && facet.Value != null)
    {
        return TypeUsage.CreateDateTimeOffsetTypeUsage(StoreTypeNameToStorePrimitiveType["timestamptz"], (byte)facet.Value);
    }
    else
    {
        return TypeUsage.CreateDateTimeOffsetTypeUsage(StoreTypeNameToStorePrimitiveType["timestamptz"], null);
    }
case PrimitiveTypeKind.Time:
    if (edmType.Facets.TryGetValue(PrecisionFacet, false, out facet) &&
        !facet.IsUnbounded && facet.Value != null)
    {
        return TypeUsage.CreateTimeTypeUsage(StoreTypeNameToStorePrimitiveType["interval"], (byte)facet.Value);
    }
    else
    {
        return TypeUsage.CreateTimeTypeUsage(StoreTypeNameToStorePrimitiveType["interval"], null);
    }
@franciscojunior
Npgsql member

I received a mail from Daniel Chafee where he talks about this issue:


It looks as if there is already an existing github issue that explains it exactly.

#11

I also verified it on my own by creating my own small command line program which simply inserts a single value into a table. The postgres column type is timestamp with timezone. I didnt post this on github because the existing example fairly well describes it i believe.

        NpgsqlConnection conn = new NpgsqlConnection("Server=xx.xx.xx.xx;User id=xxx;Port=5432;password=xxx;Database=xxx");
        conn.Open();
        var purchase_time = DateTimeOffset.Now;
        Console.WriteLine("Before Insertion: " + purchase_time);
        var insertQuery = @"INSERT INTO ""tztest"" (""purchase_time"") VALUES :purchase_time";
        var uploadTimeParam = new NpgsqlParameter("purchase_time", NpgsqlDbType.TimestampTZ);
        uploadTimeParam.Value = purchase_time;
        var insertCommand = new NpgsqlCommand(insertQuery, conn);
        insertCommand.Parameters.Add(uploadTimeParam);
        insertCommand.ExecuteNonQuery();
        var selectCommand = new NpgsqlCommand("select * from tztest", conn);
        NpgsqlDataReader dr = selectCommand.ExecuteReader();
        while (dr.Read())
        {              
           Console.Write("Retreived from database: {0} \t", dr[0]);
        }
        conn.Close();

The Console output is:

Before Insertion:             8/9/2014 8:08:49 AM -04:00
Retreived from database: 8/9/2014 4:08:49 AM

As you can see the time i put in and the time i get back out are off by 4 hours(my offset). The issue is that the offset is dropped before insertion into the DB so postgres assumes it is 8:08:49 +00 and stores it that way. Upon retrieval it will then convert to my local time as it should and it will apply a -4:00 to 8:08:49 resulting in 4:08:49 . Really this is just bad to drop the offset for this column type.

A coworker of mine applied a patch to our code base in Npgsql\NpgsqlTypes\ NpgsqlTypesHelper.cs on line 655

yield return
    //new NpgsqlBackendTypeInfo(0, "timestamptz", NpgsqlDbType.TimestampTZ, DbType.DateTime, typeof (NpgsqlTimeStampTZ),
    //                              ExtendedBackendToNativeTypeConverter.ToTimeStampTZ,
    //                              typeof(DateTime),
    //convertProviderToFramework         timestamptz => ((DateTime)(NpgsqlTimeStampTZ)timestamptz).ToLocalTime(),
    //convertFrameworkToProvider        npgsqlTimestampTZ => (npgsqlTimestampTZ is DateTime ? (NpgsqlTimeStampTZ)(DateTime)npgsqlTimestampTZ : npgsqlTimestampTZ is DateTimeOffset ? (NpgsqlTimeStampTZ)(DateTimeOffset)npgsqlTimestampTZ : npgsqlTimestampTZ));

    new NpgsqlBackendTypeInfo(0, "timestamptz", NpgsqlDbType.TimestampTZ, DbType.DateTimeOffset, typeof(NpgsqlTimeStampTZ),
                            ExtendedBackendToNativeTypeConverter.ToTimeStampTZ,
                            typeof(DateTimeOffset), 
                            timestamptz => ((DateTimeOffset)(NpgsqlTimeStampTZ)timestamptz).ToLocalTime(),
                            npgsqlTimestampTZ => (npgsqlTimestampTZ is DateTime ? (NpgsqlTimeStampTZ)(DateTime)npgsqlTimestampTZ : npgsqlTimestampTZ is DateTimeOffset ? (NpgsqlTimeStampTZ)(DateTimeOffset)npgsqlTimestampTZ : npgsqlTimestampTZ));

This appears to fix our immediate problem however we do lose some microsecond precision. What are your thoughts on incorporating this into the next build of npgsql so we don't have to maintain our own fork internally?


@jbcooley , what do you think about this patch? I remember in the past we had some discussions about this change to DateTimeOffset and backward problems it could give to users. Do you think this is still the case?

@jbcooley
@Emill
Npgsql member
@franciscojunior
Npgsql member

I thought DateTimeOffset was already mapped to timestamptz... Is there some inconsistency?

From the discussion we had in the past, the problem is that DateTimeOffset was only made available in 3.5. Later it was ported to 2.0 sp1. As we already returned DateTime, we ended up keeping returning it in order to not break backwards compatibility.

I thought DateTimeOffset was already mapped to timestamptz... Is there some inconsistency?

It is mapped from DateTimeOffset to timestamptz, but the other way around it is mapped to DateTime.

@asymetrixs

Ok, but the correct behavior would be to have it both ways DateTimeOffset <-> timestamptz.
This would fix an inconsistency and would make Npgsql work correctly with .NET >2.0.
As far as I see, there are already people (like me) mapping timestamptz <-> DateTimeOffset by hand somehow and we need to patch the code every time we upgrade Npgsql to a new version which is odd.
Can that be implemented in 3.0 and when will 3.0 be released?

@roji roji added this to the 2.3 milestone Aug 14, 2014
@franciscojunior
Npgsql member

Well done, @danbarua !

Do you think you could create a pull request with your latest patches, please?
This would be very helpful as others would be able to integrate your changes and test them.

Thanks in advance.

@franciscojunior
Npgsql member

@asymetrixs , please, also create a pull request with your changes. Your providermanifest changes are also welcome.

@danbarua

I'm sorry I have been sitting on this for a while, but this recent flurry of discussion on the issue jogged my memory!

@franciscojunior
Npgsql member

I'm sorry I have been sitting on this for a while, but this recent flurry of discussion on the issue jogged my memory!

:)

Thanks for your pull request!

Would you mind to elaborate about the Amsterdam timezone problem you talk about in your pull request? This would help us a lot!

Thanks in advance.

@danbarua

@franciscojunior i mentioned it earlier in the thread:

I've played with it and almost got it working but then I came up against the following edge case:

Note that before 1940 the time zone is not just a hour offset but a hour:minute:(and prior to 1937 even second) offset.

http://lists.pgfoundry.org/pipermail/npgsql-devel/2010-January/001063.html
http://postgresql.1045698.n5.nabble.com/npgsql-Npgsql2-Added-a-test-for-Europe-Amsterdam-timezone-with-dates-td2112154.html

.Net's DateTimeOffset class will only accept an offset in whole minutes, and will fail for DateTimes in Amsterdam up to 1937.

I think if .Net 2.0 support is being dropped then we can ignore this edge case as per the author's post.

@danbarua

Specifically, prior to 1937, Amsterdam's GMT offset was 00:19:32.13 - try passing that into the constructor of a DateTimeOffset and watch it explode.

@franciscojunior
Npgsql member

Whooops, sorry for that, @danbarua !

I thought it was another issue. :)

And thank you for your feedback on this issue. It is very enlightening!

With Josh's blessing, I vote for the inclusion of #321 to our next 3.0 release.
Shay already added it to 3.0 milestone.

@jbcooley
@franciscojunior
Npgsql member

Just for reference, I just noticed there is a project about date and time handling in .net called NodaTime: http://nodatime.org/

Maybe when Npgsql implement the idea of custom data type mappers, we could have support for date time handling with this library as well.

@roji
Npgsql member

@franciscojunior, I have't been following this in detail... It seems that now that we dropped support for .NET 2.0 and 3.5, PR #321 can be merged, right?

@franciscojunior
Npgsql member

@franciscojunior, I have't been following this in detail... It seems that now that we dropped support for .NET 2.0 and 3.5, PR #321 can be merged, right?

Yes. +1 to go ahead and merge it.

Regarding the timezone discussion, I saw this recently discussion about timezone in Postgresql mailing list which seems very enlightening:

http://postgresql.1045698.n5.nabble.com/Why-data-of-timestamptz-does-not-store-value-of-timezone-passed-to-it-td5816703.html

Here, the author says how it would be easier (for jdbc and Npgsql) if there was a data type which behaved like timestamp + offset: http://postgresql.1045698.n5.nabble.com/Why-data-of-timestamptz-does-not-store-value-of-timezone-passed-to-it-tp5816703p5817105.html

@roji
Npgsql member

I think we're ready to merge this but I just want to make sure I fully understand this complicated situation :)

Amsterdam time before 1937 specifies timezone offsets in seconds, and this isn't supported by any of the .NET types (DateTime, DateTimeOffset), right? So if it is the user's requirement to be able to handle these old Dutch dates, is there currently a way for them to request to receive NpgsqlTimeStampTZ instead of DateTimeOffset? The documentation for the "Use Extended Types" connstring param isn't entirely clear on this...

IMHO, ideally we should:

  • Provide a very specific connstring param (UseNpgsqlTimeStampTZ=true) that manages this situation. "Use Extended Types" seems very general.
  • Catch the ArgumentException generated by these old Dutch dates and produce a more informative exception explaining the situation (and telling them to turn on NpgsqlTimeStampTZ if they want)

Either way an [Ignore] should be added to the failing Amsterdam timezone test.

I might have misunderstood things though, feel free to correct :)

@Emill
Npgsql member

The "little" problem we have is that the PostgreSQL timestamptz only store the datetime value (in UTC) and not which time zone value was entered. When PostgreSQL is parsing a timestamptz, it immediately converts the value to UTC and discards the time zone. When it sends a timestamptz value, it is simply written as a string after converting to local time with the db's configured time zone.

So we can't really store a full DateTimeOffset in the database using a timestamptz field. After obtaining a timestamptz value from the database, the offset will always be the (db's) local offset.

When I test the example in a comment above:

    NpgsqlConnection conn = new NpgsqlConnection("Server=xx.xx.xx.xx;User id=xxx;Port=5432;password=xxx;Database=xxx");
    conn.Open();
    var purchase_time = DateTimeOffset.Now;
    Console.WriteLine("Before Insertion: " + purchase_time);
    var insertQuery = @"INSERT INTO ""tztest"" (""purchase_time"") VALUES :purchase_time";
    var uploadTimeParam = new NpgsqlParameter("purchase_time", NpgsqlDbType.TimestampTZ);
    uploadTimeParam.Value = purchase_time;
    var insertCommand = new NpgsqlCommand(insertQuery, conn);
    insertCommand.Parameters.Add(uploadTimeParam);
    insertCommand.ExecuteNonQuery();
    var selectCommand = new NpgsqlCommand("select * from tztest", conn);
    NpgsqlDataReader dr = selectCommand.ExecuteReader();
    while (dr.Read())
    {              
       Console.Write("Retreived from database: {0} \t", dr[0]);
    }
    conn.Close();

I get the correct time. The time is the same before and after retreival...

@roji
Npgsql member

Reading up on Postgresql TIMESTAMPTZ type, @emill seems to be right - the date is stored as UTC only without any offset - the only thing the type provides is conversion at insert-type. Since DateTimeOffset includes an offset, it is not possible for TIMESTAMPTZ to fully represent it. Follow this thread in the Postgresql mailing list, it's very relevant and suggests a composite type (i.e. two columns) to fully represent a timestamp along with its offset.

I admit I'm getting a little confused here. What was the original motivation for switching from DateTime to DateTimeOffset?

Regardless of all of this, Npgsql should be able to accept DateTimeOffset parameters when updating TIMESTAMP and TIMESTAMPTZ types... I hope this is the case...

@roji
Npgsql member

It seems like the current behavior of Npgsql is the more correct one (returning DateTime and not DateTimeOffset). If nobody has anything to add I'll close?

@franciscojunior
Npgsql member

I admit I'm getting a little confused here. What was the original motivation for switching from DateTime to DateTimeOffset?

I think we all thought timestamptz stored a more complete information which the DateTimeOffset would benefit from.

It seems like the current behavior of Npgsql is the more correct one (returning DateTime and not DateTimeOffset). If nobody has anything to add I'll close?

I think we will need to review our documentation to talk about that and explain how to better use timestamptz and datetime objects with Npgsql so our users don't think Npgsql or Postgresql or .Net isn't handling correctly their data.

@roji
Npgsql member

I think we will need to review our documentation to talk about that and explain how to better use timestamptz and datetime objects with Npgsql so our users don't think Npgsql or Postgresql or .Net isn't handling correctly their data.

I agree. I think a FAQ section would do well for this kind of thing.

Leaving open for comments for another couple of days...

@roji roji closed this Sep 17, 2014
@roji roji removed this from the 3.0 milestone Oct 14, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment