Npgsql 6.0.0-rc.1 DateTime issue

[3ldar]

We are using a library named Hangfire.PostgreSql and it uses Dapper under the hood. In that library there is a code block like below :

 using (var transaction = connection.BeginTransaction(IsolationLevel.RepeatableRead))
                {
                    int affected = -1;

                    affected = connection.Execute($@"DELETE FROM ""{options.SchemaName}"".""lock"" WHERE ""resource"" = @resource AND ""acquired"" < @timeout",
                        new
                        {
                            resource = resource,
                            timeout = DateTime.UtcNow - options.DistributedLockTimeout
                        });

                    transaction.Commit();
                }

DDL that generates the target table is like below :

create table hangfire.lock
(
    resource text not null
        constraint lock_resource_key
            unique,
    updatecount integer default 0 not null,
    acquired timestamp with time zone
);

If we change timeout = DateTime.UtcNow - options.DistributedLockTimeout into this timeout = DateTimeOffset.UtcNow - options.DistributedLockTimeout it works as expected.

In 6.0.0-rc1.0 npgsql there is a breaking change that changes the target type of DateTime into timestamp with time zone. Related info

The library uses 2.0.78 version of dapper but I've managed the reproduce the same issue with 2.0.90 version.

[NickCraver]

I'm trying to understand what the issue is - as I understand it, it looks like if the behavior has changed, the Hangfire would need to change to match. What would Dapper do differently here?

[3ldar]

I'm trying to understand what the issue is - as I understand it, it looks like if the behavior has changed, the Hangfire would need to change to match. What would Dapper do differently here?

The issue is when I try to execute the above statement with the same parameter types and values using raw Npgsql it doesn't fail. Here is the sample that succeeds

var connection = new NpgsqlConnection("cs");
await connection.OpenAsync();
using var transaction = await connection.BeginTransactionAsync(IsolationLevel.RepeatableRead);
 var command = new NpgsqlCommand($@"DELETE FROM ""hangfire"".""lock"" WHERE ""resource"" = @resource AND ""acquired"" < @timeout")
            {
                Connection = connection,
                Transaction = transaction
            };

command.Parameters.Add(new NpgsqlParameter("@resource", "abuzer"));
command.Parameters.Add(new NpgsqlParameter("@timeout", DateTime.UtcNow - TimeSpan.FromMinutes(10)));

await command.ExecuteNonQueryAsync();

[rezabay]

It seems that Dapper does not set Kind property to DateTimeKind.Utc when dealing with DateTime. Based on the new breaking changes in npgsql, Kind must be set to DateTimeKind.Utc to use TimeStampzHandler: https://github.com/npgsql/npgsql/blob/3d2a0af5c6d994bb2efad3bf6f456603b077d607/src/Npgsql/TypeMapping/BuiltInTypeHandlerResolver.cs#L459

[mgravell]

Is this for inputs? Or outputs? If inputs: that seems like something consumer code should be doing, after evaluating what the data represents. If Dapper was able to just blindly convert to UTC, then I would expect Npgsql would already be doing the same internally, so it wouldn't be an issue. If outputs: we don't creat the value - the ADO.NET provider does. Or are we talking about some more specific niche case here?

[rezabay]

This particular example is for inputs and I know for sure that the passed value is indeed DateTime with UTC kind. Npgsql does not do any conversion anymore and instead picks the corresponding time handler based on "Kind: value: https://github.com/npgsql/npgsql/blob/3d2a0af5c6d994bb2efad3bf6f456603b077d607/src/Npgsql/TypeMapping/BuiltInTypeHandlerResolver.cs#L459. The workaround is to pass DateTimeOffset but I just want to find the root cause of issue before the release date of .Net 6 but most likely the bug is on Npgsql side.

[3ldar]

Well, I have located the actual issue but have no idea how can it be resolved. I had to read that dam IL code and it basically does this :

var p = command.CreateParameter();
p.ParameterName = "@timeout";
p.Direction = ParameterDirection.Input;
p.Value = DateTime.UtcNow;
p.DbType = LookupDbType() // in this case this returns DateTime because it is explicitly set in that typeMap

But if you create the parameter without specifying any DbType NpgSql sets the DbType to DateTimeOffset

Edit I have removed the code that sets the DbType explicitly and it worked. But you have some reason to set this explicitly. It would be nice where you can override the Type -> DbType mapping.

[rezabay]

Ok, I think I kind of know what is going on. When DbType is set to DbType.DateTime, Npgsql maps it to NpgsqlDbType.Timestamp (https://github.com/npgsql/npgsql/blob/dba60fb5a87bc6d661bbdcd2a31be687edfc10e9/src/Npgsql/TypeMapping/GlobalTypeMapper.cs#L435) and later in NpgsqlParameter class (https://github.com/npgsql/npgsql/blob/dba60fb5a87bc6d661bbdcd2a31be687edfc10e9/src/Npgsql/NpgsqlParameter.cs#L505), TimestampHanler is selected instead of TimestampZHanlder resulting the issue above. I don't know if this is the expected behaviour or a bug after DateTime breaking changes.

[roji]

Hey everyone, I want to do my best to help - Npgsql 6.0 indeed does some pretty serious breaking changes. In case you're not aware, you can set an AppContext compatibility switch to revert to the pre-6.0 behavior.

I can see the issue which @3ldar is describing; Dapper sets the DbType on the parameter to the result of LookupDbType, which returns DbType.DateTime. PostgreSQL has two timestamp types: timestamp without time zone (for local/unspecified) and timestamp with time zone(for UTC); Npgsql uses DbType.DateTime to refer to the former and DbType.DateTimeOffset to refer to the latter (since there's no datetimeoffset DB type). Maybe DbType.DateTime2 would be a better choice (or in addition), but that's beside the point.

The thing is, in 6.0, we no longer allow UTC timestamps to be written to timestamp without time zone, since that's mixing timestamp types (UTC goes to UTC, non-UTC to non-UTC). If DbType were to be unset, Npgsql would simply infer automatically what to do by looking at the DateTime's Kind, but since Dapper sets it explicitly, it just does what it's told...

@mgravell @NickCraver is there a particular reason to set DbType rather than let the driver decide? I'm open to making a change on the Npgsql if absolutely necessary, but would like to understand if Dapper is doing the right thing etc.

[mgravell]

Hi @roji - I'm pretty open to changing it to not specify anything, if that is going to work better. Date time is kinda unique among the primitives in that we keep inventing new ways of handling it. If omitting a type entirely is going to work better for dates/times: that's fine! I can probably try that tomorrow if you like.

[roji]

For Npgsql it's definitely better - just hoping there's not some reason it was done this way etc. Basically the ADO.NET driver is in a better position to know what to write based on the CLR type than Dapper is...

[roji]

BTW another possibly problematic one is TimeSpan -> DbType.Time... PostgreSQL has an interval type (which can be over 1 day), and a time type (which is really for time of day). Npgsql maps TimeSpan to interval (since TimeSpan can be more than a day), but DbType.Time maps to time... so you're effectively overriding the "natural" provider mapping. BTW luckily .NET now has TimeOnly, which can be cleanly mapped to time...

Type mapping is hard... especially date/times...

[mgravell]

spike started, @roji - should be on MyGet soon, assuming no explosions; what would we need to repro the failure here, to prove the fix?

[mgravell]

huh; tell a lie - looks like we only build MyGet for merges, but: it built and tested cleanly, at least

[roji]

The following fails with Npgsql 6.0.0-rc.1:

_ = conn.ExecuteScalar("SELECT @Now", new { Now = DateTime.UtcNow });

If you avoid setting DbType, Npgsql should internally infer the right PostgreSQL type and all should be well.

... and thanks for spending time on this!!

[mgravell]

building with that now: https://ci.appveyor.com/project/StackExchange/dapper/builds/41146676

[mgravell]

well, the good news is that the new TestPostgresqlDateTimeUsage passes; the less good news is that .TestPostgresqlArrayParameters now fails:

        [FactPostgresql]
        public void TestPostgresqlArrayParameters()
        {
            using (var conn = GetOpenNpgsqlConnection())
            {
                IDbTransaction transaction = conn.BeginTransaction();
                conn.Execute("create table tcat ( id serial not null, breed character varying(20) not null, name character varying (20) not null);");
                conn.Execute("insert into tcat(breed, name) values(:breed, :name) ", Cats);

                var r = conn.Query<Cat>("select * from tcat where id=any(:catids)", new { catids = new[] { 1, 3, 5 } });
                Assert.Equal(3, r.Count());
                Assert.Equal(1, r.Count(c => c.Id == 1));
                Assert.Equal(1, r.Count(c => c.Id == 3));
                Assert.Equal(1, r.Count(c => c.Id == 5));
                transaction.Rollback();
            }
        }

[roji]

Uhh... I don't know enough about what's going on internally to know... I'm assuming this change does just remove the setting of DbType, right? FWIW for arrays there's no correct DbType anyway...

[mgravell]

should be; checking to see whether I borked anything; never underestimate my ability to bork things - it is unrivaled

(FWIW, the same code worked on npgsql v5)

[3ldar]

@mgravell can you try it with a List<T>.

[mgravell]

I don't have npgsql locally, so I'm a little dependent on CI, but: List<int> failed too; not sure what is up without being able to debug: https://ci.appveyor.com/project/StackExchange/dapper/builds/41147028/tests

[3ldar]

@mgravell Sadly it works on my version of the code (Where setting DbType is completely omitted). And also https://github.com/DapperLib/Dapper/commit/23ab4ddda6907143ef1680f02787402ea7bf6439 is also working on my local.

[mgravell]

@3ldar to confirm: you're saying that the tests shown all pass for your locally? arrays and lists?

[3ldar]

@3ldar to confirm: you're saying that the tests shown all pass for your locally? arrays and lists?

Well, In a way yes, I crated that table on my local populated it with data. And executed it the exact way in the test.

[roji]

@mgravell are you able to isolate the array case to the problematic SQL (and parameter config) being generated? If so I can maybe try to help from there...

[mgravell]

hypothetically ... just wondering; what would happen if .DbType = (DbType)-1;

yes, obviously nobody would do that because it is stupid, but... humour me... would that break?

[roji]

Uhh.... Yeah, that won't work... Since DbType is set, Npgsql will still attempt to resolve the PG type through that, and not by looking at the CLR type. And then we get an exception because we don't know that DbType... I could make Npgsql check for that and fall back to CLR type inference, but that seems like squishy behavior that ignores user error... How is not setting the DbType responsible for modifying the array behavior?

[mgravell]

it is a hack that we can fix; basically, when trying to resolve how to handle args, we call a LookupDbType method, which historically always returned a DbType or threw; that's the API that I changed to return DbType? to return null for DateTime etc, interpreting null as "don't set", but I'm wondering if what is happening here is that we're recognizing that this data is enumerable and returning the sentinel that we use for some other scenarios (basically, to unroll execute parameters and auto-loop). That sentinel is -1.

My hypoethesis, then, is that we should change:

if (dbType.HasValue) param.DbType = dbType.GetValueOrDefault();

to something similar which also excludes -1

Trying now...

[roji]

Ah I see... FWIW Npgsql specifically doesn't support enumerables, only actual arrays and List<T> (since enumerables would need to be enumerated more than once). If the array support is purely for PG (which I suspect it is), you could also narrow your array detection to correspond to that...

[mgravell]

we could special-case it, but in reality: there is no valid scenario where we should be setting the DbType to a nonsensical value, so: the fact that we've gotten away with it until now is largely moot; testing now

[KinNeko-De]

I have a similar issue with testing Npgsql 6.0.0-rc.2 Dapper version 2.0.90

I have a column with a 'timestamptz' format and I want to add a C# 'DateTime' of Kind 'UTC' into this column. The DateTime is generated by DateTime.UtcNow. I have a working example with pgsql 5.x.x

When I insert data to this table using dapper I get the following error message: 'Cannot write DateTime with Kind=UTC to PostgreSQL type 'timestamp without time zone', consider using 'timestamp with time zone'. Note that it's not possible to mix DateTimes with different Kinds in an array/range. See the Npgsql.EnableLegacyTimestampBehavior AppContext switch to enable legacy behavior.'

As far as I understand, this is also related to the DbType. I think it is currently 'timestamp" for DateTime and Npgsql 6.0 rejects that now.

I have inserts with "copy" without dapper that still work. The only reason I test this feature is to get a UTC Datetime on select without registering using a specific dapper datetimehandler. Selecting using dapper also works fine for me. EntityFramework had a similar problem with inserting data. https://github.com/zzzprojects/EntityFramework-Extensions/issues/441

So I will not create another issue for that.

[roji]

@KinNeko-De it sounds like you're running into the same issue already described here. I know @mgravell is working on a solution.

[3ldar]

Thanks, everyone for the effort, happy to close this out.

[mgravell]

Build for this went to nuget yesterday, btw.

[pdevito3]

PostgreSQL has two timestamp types: timestamp without time zone (for local/unspecified) and timestamp with time zone(for UTC); Npgsql uses DbType.DateTime to refer to the former and DbType.DateTimeOffset to refer to the latter (since there's no datetimeoffset DB type).

@roji is this flipped? I just ran a migration on a DateTime prop and it was scaffolded as 'timestamp with timezone' like so: last_modified_on = table.Column<DateTime>(type: "timestamp with time zone", nullable: true),. I got to this issue with my system throwing the original error above about assigning non-UTC to UTC because I was doing DateTime.Now instead of DateTime.UtcNow. I really like this new enforcement, but just wanted to double check I'm understanding it correctly.

[roji]

@pdevito3 the interpretation of DbType changed for the final release, see https://github.com/npgsql/npgsql/issues/4106 for more details; tl;dr:

DbType.DateTime -> timestamp with time zone DbType.DateTime2 -> timestamp without time zone DbType.DateTimeOffset -> timestamp with time zone

However, that isn't related to the migration change you're describing above (which I'm assuming is EF Core). EF Core doesn't care about DbType, and Dapper no longer sets DbType for DateTime. In the Npgsql EF Core provider, DateTime now maps to timestamp with time zone by default, so you can no longer assign non-UTC DateTime to it (hence DateTime.UtcNow and not DateTime.Now).

[pdevito3]

yeah, EF Core thanks. makes sense. sorry for polluting the dapper thread!