Feature Request: Per-connection Npgsql.EnableLegacyTimestampBehavior

[mwpastore]

Steps to reproduce

• Create NpgsqlConnection "A" where the business logic requires Npgsql.EnableLegacyTimestampBehavior set to true
• Create NpgsqlConnection "B" where the business logic requires Npgsql.EnableLegacyTimestampBehavior set to false

The issue

Depending on the order of operations, sometimes A will have Npgsql.EnableLegacyTimestampBehavior set to false, and sometimes B will have Npgsql.EnableLegacyTimestampBehavior set to true. It is difficult to set this switch deterministically and even more difficult to test. This creates a serious conflict in our app that uses both types of connections.

Instead we'd like a way to configure each Npgsql connection with the appropriate value of Npgsql.EnableLegacyTimestampBehavior.

Further technical details

• Npgsql version:
  • A is using 8.0.0
  • B is using 6.0.0
• PostgreSQL version: 13
• Operating system: Ubuntu Linux

[roji]

@mwpastore I'm trying to understand your scenario, can you provide a bit more context?

First, am I understanding correctly that the same application is using two different versions of Npgsql (6.0 and 8.0)? That is definitely not a typical thing, and not necessarily well-supported.

Second, above you seem to be saying that the difficulty to set the switch deterministically is the reason for asking for a per-connection setting. That's not very clear to me; either your program is written to work with the legacy behavior, or with the newer behavior - so which is it? Or are you saying you have different components, where some have been modified to use the new behavior but not all?

[mwpastore]

@roji

I'm trying to understand your scenario, can you provide a bit more context?

Thank you for the quick response!

First, am I understanding correctly that the same application is using two different versions of Npgsql (6.0 and 8.0)? That is definitely not a typical thing, and not necessarily well-supported.

I could be wrong; I'm looking through packages.lock.json to try to figure out how NuGet is resolving our dependencies. Our app depends on Npgsql.EntityFrameworkCore.PostgreSQL 8.0.0, which depends on Npgsql 8.0.0 (A); and our app depends on Hangfire.PostgreSql 1.20.5, which depends on Npgsql 6.0.0 (B).

Or are you saying you have different components, where some have been modified to use the new behavior but not all?

Yes, we have a very large codebase of our own that relies on the old behavior (A), and we are also using Hangfire.PostgreSql, which relies on the new behavior (B). Two different databases, two different connection strings, etc., but the EF connection pool needs Npgsql.EnableLegacyTimestampBehavior set to true, and the Hangfire.PostgreSql connection pool needs Npgsql.EnableLegacyTimestampBehavior set to false.

[roji]

@mwpastore thanks for the additional details. First, note that Hangfire.PostgreSql indeed seems to depend on Npgsql >= 6.0, but since you're taking a direct dependency on a higher version (8.0), that should simply get chosen. In other words, your application is simply using Npgsql 8.0 (no 6.0 anywhere), as nuget resolves to the lowest version supported by all references.

Unfortunately, I don't really see us introducing a per-connection Npgsql.EnableLegacyTimestampBehavior... The change has been made a long time ago, and generally users have managed to update their codebases to use the new behavior since then; while I can appreciate that this can be an effort, adding another connection string parameter to control this could have perf repercussions, and we're generally averse to adding this kind of knob. Another option is to downgrade to an older version of Hangfire.PostgreSql which doesn't require the new behavior, until you manage to upgrade.

[roji]

@mwpastore I discussed internally, and we'll keep this issue open for now; although we haven't seen much request for this (and 6.0 was released around 2 years ago), if other users request this we'll consider it. In the meantime I strongly recommend planning for modifying your code to use the new behavior - that's generally a better and safer way.

[schmitch]

The change has been made a long time ago, and generally users have managed to update their codebases to use the new behavior since then; while I can appreciate that this can be an effort, adding another connection string parameter to control this could have perf repercussions, and we're generally averse to adding this kind of knob. Another option is to downgrade to an older version of Hangfire.PostgreSql which doesn't require the new behavior, until you manage to upgrade.

@roji I really doubt that this is the case. the migration is as painful as it gets and there are probablly users on .net 6 (my company has 3 apps without the migration)

and there are people who still find bugs for this feature, so I think there are still some users on the old behavior.

[roji]

and there are people who still find bugs for this feature

There have been a few, yes - I wouldn't say it's an indication of massive usage either. In any case, the global backwards compatibility switch is there and we don't intend to remove it any time soon; but I also haven't seen a real justification for introducing a per-connection setting either.

[newclaus]

I have a similar situation – two contexts with different behaviors. And when I updated to Npgsql 9, an error appeared during migration application that there are pending changes. I have no clue how to work around this to be sure that everything is going to work ok.
It's also unclear why you wouldn't include this setting in the builder since the new version introduced breaking changes with SSL configuration in it.

[vonzshik]

It's also unclear why you wouldn't include this setting in the builder since the new version introduced breaking changes with SSL configuration in it.

Pretty much because of the way parameters work. Specifically, whenever you set NpgsqlParameter.DbType we also have to set NpgsqlParameter.NpgsqlDbType because that's the exact same behavior SqlClient has. And depending on whether the legacy is enabled, DbType.DateTime will return either timestamp or timestamptz.

npgsql/src/Npgsql/NpgsqlTypes/NpgsqlDbType.cs

Line 585 in 8effce1

DbType.DateTime => LegacyTimestampBehavior ? NpgsqlDbType.Timestamp : NpgsqlDbType.TimestampTz,

And since NpgsqlParameter has no idea about the connection (and the settings) it's going to be used on...

[schmitch]

@roji since the field itself is a initonly static field, it's basically impossible to have a migration project with two different configured DbContext's ? Or am I somehow mistaken?

[roji]

@schmitch that's right - that's the current situation.

[schmitch]

at the moment we use:

        AppContext.SetSwitch("Npgsql.EnableLegacyTimestampBehavior", value);
        var assembly = Assembly.GetAssembly(typeof(NpgsqlConnection));
        var npgsqlType = assembly!.GetType("Npgsql.Util.Statics", true, true)!;
        var constructor = npgsqlType
            .GetConstructor(BindingFlags.Static | BindingFlags.NonPublic,null, [], null)!;
        constructor.Invoke(null, null);

which looks kinda working, if you close the NpsqlDataSource and re-initalize everything from the beginning (which is fine for us).
I'm also pretty sure that it might fail because of inlining.

But I can also understand that it is not easily changeable, since it would basically change the whole library... It's probably easier to create a new DateTime type that can basically do the conversion, something like NodaTime, but more like a wrapper class that always contains UTC and basically works with Npgsql.EnableLegacyTimestampBehavior to get the correct internal DateTime and than the application can convert from UTC to the Local TimeZone if needed either via application code or by using a kind of default timezone that is built into the type.

[roji]

It's probably easier to create a new DateTime type that can basically do the conversion, something like NodaTime [...]

Honestly, if you're contemplating such a big such, I'd advise simply switching to NodaTime or updating your code to work with the non-legacy behavior. That's a much better investment IMHO.

[schmitch]

sadly we tried with NodaTime and it was not such a great experience, that we basically stopped the migration.