Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support Npgsql 6.0+ #216

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

PopovKS
Copy link

@PopovKS PopovKS commented Jan 27, 2022

Added support Npgsql 6.0+ driver

@alex-kulakov
Copy link
Contributor

A lot of tests are failed. At least DateTime and DateTimeOffset support is broken for all the test instances from v9.6 up to v13, basic INSERTs are broken. I cannot merge this PR.

@PopovKS
Copy link
Author

PopovKS commented Feb 6, 2022

A lot of tests are failed. At least DateTime and DateTimeOffset support is broken for all the test instances from v9.6 up to v13, basic INSERTs are broken. I cannot merge this PR.

Yes, new version of Npgsql driver throw exception if you try save DateTime without KindType.Utc to DB column with support timezone info.

Offical Postgres doc (https://www.npgsql.org/doc/types/datetime.html) contains information about this and suggest several solutions:

  1. Enable special static option for application:
    AppContext.SetSwitch("Npgsql.EnableLegacyTimestampBehavior", true);
  2. Fix all DateTime and set it right KindType

Currently all test use DateTime with KindType.Unspecified.

After enable options for support legacy behavior all test successful.

I sugest change KindType for DateTime property on UTC when parsing of test data from XML and for others instance of DateTime , it's ok for you?

@alex-kulakov
Copy link
Contributor

Enable special static option for application:
AppContext.SetSwitch("Npgsql.EnableLegacyTimestampBehavior", true);

It worked fine before so having another option to turn on to not screw application work is bad idea.

I suggest change KindType for DateTime property on UTC

Imagine you had DateTime values in database, you updated the package and all dates that had been stored before became UTC dates. That's a nightmare.

For DateTime values I suggest to use timestamp without timezone explicitly. Apparently, Npgsql uses timestamp with timezone for that by default and had broken backwards compatibility (what an idea!🤦‍♂️). But this is half of the problem, the second part is DateTimeOffset values. It should work too. I would try to switch zone for values to UTC before saving (PgSQL did it anyway on server side).

To do so take a look at TypeMapper.cs which is in all of SQL drivers. For each supported .Net type it a has trinity of methods:

  • MapXXX - maps .net type to native SQL type in form of SqlValueType;
  • BindXXX - binds value of .Net type to DbParameter
  • ReadXXX - reads values from query result row and transforms it to corresponding .Net type.

You need to change or override BindDateTime() and BindDateTimeOffset(), the second one is already overridden for sure.

If you have any questions, please ask.

By the way, what was the problem with IsTransactionCompleted()?

@PopovKS
Copy link
Author

PopovKS commented Feb 14, 2022

Ok, thanks, I will do it.

@PopovKS
Copy link
Author

PopovKS commented Mar 17, 2022

@alex-kulakov fixed mapping for DateTimeOffset and DateTime. All tests passed in master branch same way passed with new driver version in current branch

IsTransactionCompleted() use private field IsCompleted for get transaction state in early versions of npgsql driver it field was be public. Now Npgsql checks what the transaction completed internally in methods Commit() and Rollback()

@alex-kulakov
Copy link
Contributor

@PopovKS , that's weird that you fixed DateTimeOffset in v8_0.TypeMapper and DateTime - in v10_0.TypeMapper? That means no proper support for DateTime values for PostgreSQL v9.6 which is still supported PostgreSQL version.

You don't need to override all three Map, Read, Bind methods, As I understand from my quick investigation something like this

    public override void BindDateTime(DbParameter parameter, object value)
    {
      var nativeParameter = (NpgsqlParameter) parameter;
      nativeParameter.NpgsqlValue = value ?? DBNull.Value;
    }

works for all DateTime values (Unspecified and Utc kinds). Funny, huh? 😁 I did it in v8_0.TypeMapper, so no need to change v10_0.TypeMapper at all. I'll test the code above through all test storages we usually test on during today and tomorrow. Maybe some additional changes will have to be made.

IsTransactionCompleted() use private field IsCompleted for get transaction state in early versions of npgsql driver it field was be public. Now Npgsql checks what the transaction completed internally in methods Commit() and Rollback()

Got it.

@alex-kulakov
Copy link
Contributor

There is another thing that should be investigated - DateTime.Min and DateTime.Max replacing with Infinity. Maybe there are more things they wanted to break compatibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants