Upgrade npgsql (Postgres) to the very latest version #196

Bartleby2718 · 2024-03-20T02:22:24Z

Context

Per the discussion in #111, this PR upgrades Npgsql from 5.0.4 to 8.0.2, the latest version at the time of filing.

Considerations

Breaking Changes

I don't think there are breaking changes that affect DistributedLock, but please take some time to go over the breaking changes:

in Npgsql 6: https://www.npgsql.org/doc/release-notes/6.0.html#breaking-changes
in Npgsql 7: https://www.npgsql.org/doc/release-notes/7.0.html#breaking-changes
in Npgsql 8: https://www.npgsql.org/doc/release-notes/8.0.html#breaking-changes

Usage of the Recent Versions

As you can see in https://www.nuget.org/packages/Npgsql/8.0.2#versions-body-tab, 8.0.0 has the most number of downloads, but both 8.0.1 and 8.0.2 have over a million downloads. Given that these are bug patches, I think 8.0.2 is better than the previous 8.0.* versions.

Open Questions

…ion because the latest minor/patch in the current major version doesn't support CancellationToken in PrepareAsync()

Bartleby2718 · 2024-03-20T02:23:50Z

src/Directory.Packages.props

Open Question 1: In 78f3386, @madelson you built using the Release configuration and there were some changes in packages.lock.json files. This is because we have this Condition on line 8. The is the only Condition in Directory.Packages.props that doesn't use TargetFramework. Therefore, a normal build can cause a difference in packages.lock.json files based on the Configuration used for the build. Should we just get rid of this Condition?

Should we just get rid of this Condition?

I think this will cause other problems, unfortunately. If there was a way that we could install the analyzer every time and then use a condition to disable it for DEBUG that would be ideal

@madelson I did a little more research, which suggests that my proposed solution is actually a typical practice.
https://www.nuget.org/packages/Microsoft.CodeAnalysis.PublicApiAnalyzers/#usedby-body-tab lists 5 GitHub repositories that use this NuGet package:

https://github.com/search?q=repo%3Adotnet%2Fmaui+Microsoft.CodeAnalysis.PublicApiAnalyzers&type=code

https://github.com/search?q=repo%3ADapperLib%2FDapper%20Microsoft.CodeAnalysis.PublicApiAnalyzers&type=code

https://github.com/search?q=repo%3AApp-vNext%2FPolly%20Microsoft.CodeAnalysis.PublicApiAnalyzers&type=code

https://github.com/search?q=repo%3AStackExchange%2FStackExchange.Redis%20Microsoft.CodeAnalysis.PublicApiAnalyzers&type=code

https://github.com/search?q=repo%3AMessagePack-CSharp%2FMessagePack-CSharp%20Microsoft.CodeAnalysis.PublicApiAnalyzers&type=code

None of the top 5 has a Condition on Configuration (including the ItemGroups that contain the PackageReference elements).

Normally I'd have no issue with this in DEBUG, the problem is that this particular package does something weird in DistributedLock.Core to manage the pseudo-public dependencies (see for example this file). Because of that, if you run this analyzer in DEBUG it gets upset that those methods are not in the public API but they shouldn't be.

So this project is an atypical situation. Every project except DistributedLock.Core should be able to run this package in DEBUG with no issues.

If you're not convinced, you can try it and report back (put the package on in all configurations, then try command line and VS builds in both DEBUG and RELEASE configurations).

Like I said, one alternative would be to do the above, and then just conditionally suppress the warnings in DEBUG for DistributedLock.Core. That would be a nice solution if it can be made to work.

Given that we multi-target TFMs in the test project based on Configuration, we're bound to have some lock files changes based on Configuration. Therefore, I think e559c5c looks best to me. With this, the only differences between dotnet build -c Release and dotnet build -c Debug are lock files for DistributedLock.Core and DistributedLock.Tests, allowing us to minimize churns.

Bartleby2718 · 2024-03-20T02:24:01Z

src/Directory.Packages.props

@@ -23,7 +23,7 @@
    <PackageVersion Include="System.Threading.AccessControl" Version="5.0.0" Condition="'$(TargetFramework)' != 'net461'" />
    <PackageVersion Include="ZooKeeperNetEx" Version="3.4.12.4" />
    <PackageVersion Include="IsExternalInit" Version="1.0.3" />
-    <PackageVersion Include="Microsoft.Bcl.AsyncInterfaces" Version="5.0.0" Condition="'$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net461'" />
+    <PackageVersion Include="Microsoft.Bcl.AsyncInterfaces" Version="8.0.0" Condition="'$(TargetFramework)' == 'netstandard2.0' OR '$(TargetFramework)' == 'net461'" />


Open Question 2: There's this chain of transitive dependency: Npgsql 8.0.2 -> System.Text.Json 8.0.0 -> Microsoft.Bcl.AsyncInterfaces 8.0.0. However, we have Microsoft.Bcl.AsyncInterfaces 5.0.0 here, so we're getting error NU1109: Detected package downgrade: Microsoft.Bcl.AsyncInterfaces from 8.0.0 to centrally defined 5.0.0. Update the centrally managed package version to a higher version.. Therefore, I made this change, but now we see this warning: warning : Microsoft.Bcl.AsyncInterfaces 8.0.0 doesn't support net461 and has not been tested with it. Consider upgrading your TargetFramework to net462 or later. You may also set <SuppressTfmSupportBuildWarnings>true</SuppressTfmSupportBuildWarnings> in the project file to ignore this warning and attempt to run in this unsupported configuration at your own risk. [DistributedLock\src\DistributedLock.ZooKeeper\DistributedLock.ZooKeeper.csproj::TargetFramework=net461]
This brings us back to madelson/MedallionShell#113; should we continue to support net461 and other versions which already reached end of support?

Let's go to net462 throughout and see if everything is happy. My main goal is to keep supporting .NET framework for the time being, and not to drop support for specific framework versions without a reason (but this is a reason!).

@madelson AppVeyor seems happy! 🎉
Note that I didn't update any versions or release notes.

Bartleby2718 · 2024-03-20T02:25:49Z

src/DistributedLock.Azure/packages.lock.json

@@ -18,15 +18,6 @@
        "resolved": "3.3.4",
        "contentHash": "kNLTfXtXUWDHVt5iaPkkiPuyHYlMgLI6SOFT4w88bfeI2vqSeGgHunFkdvlaCM8RDfcY0t2+jnesQtidRJJ/DA=="
      },
-      "Microsoft.NETFramework.ReferenceAssemblies": {


This was added in 78f3386, but I'm not sure why this was a direct dependency. Maybe your VS is set up in a certain way?

Yeah not sure. This came in when I built. I'm OK with some thrash here.

Bartleby2718 · 2024-03-20T02:26:45Z

src/DistributedLock.Core/Internal/Data/DatabaseCommand.cs

-        // Note: for now we cannot pass cancellationToken to PrepareAsync() because this will break on Postgres which
-        // is the only db we currently support that needs Prepare currently. See https://github.com/npgsql/npgsql/issues/4209
-        await this.PrepareIfNeededAsync(CancellationToken.None).ConfigureAwait(false);
+        await this.PrepareIfNeededAsync(cancellationToken).ConfigureAwait(false);


This is the only code change. I didn't add any comments because this is the expected behavior and the original behavior was an exception.

Bartleby2718 · 2024-03-23T22:48:10Z

src/DistributedLock/DistributedLock.csproj

@@ -53,7 +53,7 @@
    <ProjectReference Include="..\DistributedLock.Redis\DistributedLock.Redis.csproj" />
    <ProjectReference Include="..\DistributedLock.ZooKeeper\DistributedLock.ZooKeeper.csproj" />
    <ProjectReference Include="..\DistributedLock.MySql\DistributedLock.MySql.csproj" />
-    <ProjectReference Include="..\DistributedLock.Oracle\DistributedLock.Oracle.csproj" Condition="'$(TargetFramework)' != 'netstandard2.0' AND '$(TargetFramework)' != 'net461'" />


TargetFrameworks is netstandard2.0;netstandard2.1;net462 in this project and netstandard2.1;net462inDistributedLock.Oracle. Therefore, we only need to filter out netstandard2.0`.

src/DistributedLock.Core/Internal/Data/DatabaseCommand.cs

madelson · 2024-03-24T14:14:20Z

This looks good to go. By my count the only outstanding small comments are:

https://github.com/madelson/DistributedLock/pull/196/files#r1536824783 (if you wanted to try the approach suggested; can also be punted to a separate issue).
https://github.com/madelson/DistributedLock/pull/196/files#r1536825713 (minor cleanup)

Bartleby2718 · 2024-03-26T01:19:45Z

Leaving a comment so @madelson can assign me.

Close madelson#111: Upgrade npgsql (Postgres) to the very latest vers…

06da5b0

…ion because the latest minor/patch in the current major version doesn't support CancellationToken in PrepareAsync()

Bartleby2718 commented Mar 20, 2024

View reviewed changes

Bartleby2718 marked this pull request as ready for review March 20, 2024 02:27

Drop support for net461 in all projects

449d7e4

Bartleby2718 commented Mar 23, 2024

View reviewed changes

Bartleby2718 requested a review from madelson March 23, 2024 22:49

madelson reviewed Mar 24, 2024

View reviewed changes

src/DistributedLock.Core/Internal/Data/DatabaseCommand.cs Outdated Show resolved Hide resolved

Bartleby2718 added 3 commits March 24, 2024 15:03

Adjust Conditions to minimize churns around lock files

e559c5c

Add missing solution items for VS users

ae4de21

Replace NET462 with NETFRAMEWORK in preprocessor directives

8366aef

Bartleby2718 requested a review from madelson March 24, 2024 21:30

madelson approved these changes Mar 24, 2024

View reviewed changes

madelson merged commit 8c10023 into madelson:release-2.4 Mar 24, 2024
1 check passed

Bartleby2718 deleted the npgsql-8.0.2 branch March 26, 2024 01:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade npgsql (Postgres) to the very latest version #196

Upgrade npgsql (Postgres) to the very latest version #196

Bartleby2718 commented Mar 20, 2024 •

edited

Loading

Bartleby2718 Mar 20, 2024

madelson Mar 23, 2024

Bartleby2718 Mar 23, 2024

madelson Mar 24, 2024 •

edited

Loading

Bartleby2718 Mar 24, 2024

Bartleby2718 Mar 20, 2024

madelson Mar 23, 2024

Bartleby2718 Mar 23, 2024

Bartleby2718 Mar 20, 2024

madelson Mar 23, 2024

Bartleby2718 Mar 20, 2024

Bartleby2718 Mar 23, 2024

madelson commented Mar 24, 2024

Bartleby2718 commented Mar 26, 2024

Upgrade npgsql (Postgres) to the very latest version #196

Upgrade npgsql (Postgres) to the very latest version #196

Conversation

Bartleby2718 commented Mar 20, 2024 • edited Loading

Context

Considerations

Breaking Changes

Usage of the Recent Versions

Open Questions

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson Mar 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madelson commented Mar 24, 2024

Bartleby2718 commented Mar 26, 2024

Bartleby2718 commented Mar 20, 2024 •

edited

Loading

madelson Mar 24, 2024 •

edited

Loading