Postgres: consider constructor with NpgsqlDatasource

[madelson]

[davidngjy]

Hey @madelson, has there been any progress on this one?
We been utilising the new NpgsqlMultiHostDataSource for our multi-region setup, load balancing across two clusters of Postgres.

This would be very helpful for us as we use the distributed lock for some scenario where we want to avoid processes overlapping each other.

[madelson]

@davidngjy for my knowledge, do Postgres locks actually work in that load balanced scenario? How does it synchronize across clusters?

This issue should be pretty straightforward to implement, I just haven’t gotten around to it yet (and no one had explicitly asked for it). Basically we would just be using the data source as a connection factory.

[davidngjy]

Hey @madelson,

Sorry, I realised that load balancing is not correct term for it. We have a single primary with multiple read replicas. Specifically, we're using AWS Aurora with two clusters (one in each region), where only one instance across the entire cluster acts as the primary.

We have a shared package leveraging your .Core package to establishing a distributed lock (we have SQL Server and Postgres). On the client side, we need to register the IDistributedLockProvider.

Following the Npgsql documentation and in combination with the provided DI package, we currently resolving the IDbConnection in the following way:

serviceCollection.AddMultiHostNpgsqlDataSource(configuration.GetConnectionString("Postgres"));

serviceCollection.AddTransient<IDbConnection>(provider =>
{
    var dataSource = provider.GetRequiredService<NpgsqlMultiHostDataSource>();
    return dataSource.WithTargetSession(TargetSessionAttributes.Primary).OpenConnection();
});

serviceCollection.AddTransient<IDistributedLockProvider>(provider => new PostgresDistributedSynchronizationProvider(provider.GetRequiredService<IDbConnection>()));

While this works, the icky part is the line
return dataSource.WithTargetSession(TargetSessionAttributes.Primary).OpenConnection();
where it's resolving an asynchronous task using the synchronous method. I'm not sure how significant is the operation but I assume this operation identifies the primary instance using the provided multi-host connection string since the returned NpgsqlDataSource that correctly points to the active instance.

It would be nice to have the PostgresDistributedSynchronizationProvider take in the NpgsqlDatasource, allowing the registration to be simplified and avoid any asynchronous operation within the DI

serviceCollection.AddTransient<NpgsqlDataSource>(provider =>
{
    var dataSource = provider.GetRequiredService<NpgsqlMultiHostDataSource>();
    return dataSource.WithTargetSession(TargetSessionAttributes.Primary);
});

serviceCollection.AddTransient<IDistributedLockProvider>(provider => new PostgresDistributedSynchronizationProvider(provider.GetRequiredService<NpgsqlDataSource>()));

Maybe even have the library to have the option for NpgsqlMultiHostDataSource?

serviceCollection.AddTransient<IDistributedLockProvider>(provider => new PostgresDistributedSynchronizationProvider(provider.GetRequiredService<NpgsqlMultiHostDataSource>()));

Not sure if it's too opinionated but the lock must operate on the Primary instance?

[davidngjy]

Hey @madelson, has any work been done on this one yet?
If not, I would like to give this one a go if it's open for contribution, any guidance or design you have in mind would be appreciated!

[madelson]

Go for it @davidngjy !

as far as advice, if you follow the code flow for the connection string constructor you can see that on one path we pretty quickly get to creating a connection factory function which would be easy to create with the datasource’s create connection function as well.

what would be trickier is if we want to support multiplexing in this configuration. That would require changes to the multiplexing infra in core. Up to you if you want to look into that; the first version could make the new constructor incompatible with multiplexing.

as far as API, I would expect that the only change is a new constructor on the lock classes which takes DBDatasource instead off connection string.

[davidngjy]

Hey @madelson,

I made my first attempt at implementing support for NpgsqlDataSource and would appreciate your early feedback to ensure I'm on the right track before I add test coverage and address any missing parts.
Here's the link to the PR: #214.

One thing I'm unsure about is the isExternallyOwned flag in the PostgresDatabaseConnection class. In a quick test, I found that the CanExecuteQueries flag in the DatabaseConnection base class returns a false if the client doesn't call OpenConnectionAsync().
To allow the library to open the connection, isExternallyOwned needs to be set to false.

Would my assumption be correct that we should manage the connection internally when a NpgsqlDataSource is provided?

Also, I'm unsure about the merging strategy, please guide me through this.
Looking forward to your thoughts and feedback. Thanks!

[madelson]

LGTM so far; left a couple comments on the PR so please take a look. A few more thoughts:

isExternallyOwned

This flag indicates who is owns the connection. A connection provided by a library user is externally owned so the library cannot close it and can only access it when the user specifically requests. In this case, the library is creating the connection via the data source so it is correct for isExternallyOwned to be false.

DbDataSource

To use DbDataSource we probably need to add #if branches and a net7 build target for the Postgres project. If you were really hoping to use this constructor on older .NET versions, let's talk about that since I think there is a path but I'm less excited about it.

[davidngjy]

Hey @madelson,

Thanks for the explanation!

I did encounter some build issues when referencing DbDataSource directly, but those go away when I add a conditional branch for net7 or higher.

One issue I have is that when I add the if branch (see screenshot below), the whole block is greyed out, and I lose the analyzer. However, I can still write tests that target the specific constructor just fine.
I only have experience working with applications that target a specific framework (this is my first OSS contribution 😀).
Let me know if there's a better way to improve the development experience.

Most of our apps are on .NET 8 now, with just a few on .NET 7, so we are all good!

Let me know if I made the changes correctly in the latest push. If so, the next step would be to add test coverage. I would appreciate any guidance on where to start or if there's a reference I could refer to!

[davidngjy]

Hey @madelson,

I made some updates to the PR.

The options = options is null ? o => o.UseMultiplexing(false) : o => { o.UseMultiplexing(false); options(o); }; code results in a recursive call that never ends so I have to change up a bit to make it work. Let me know if you have a better idea.

I've added some tests, they're mainly checking the nullability and the multiplexing configuration. Not sure if there's anything else required.

I keep getting the following errors

/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/PostgresAdvisoryLockKey.cs(99,26): error CS8765: Nullability of type of parameter 'obj' doesn't match overridden member (possibly because of nullability attributes). [/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/DistributedLock.Postgres.csproj::TargetFramework=net8.0]

Didn't touch this method, not sure why it's showing up suddenly.

/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/PostgresDistributedSynchronizationProvider.cs(43,12): error RS0026: Symbol 'PostgresDistributedSynchronizationProvider' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details. (https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md) [/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/DistributedLock.Postgres.csproj::TargetFramework=net8.0]
/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/PostgresDistributedLock.cs(39,12): error RS0026: Symbol 'PostgresDistributedLock' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details. (https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md) [/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/DistributedLock.Postgres.csproj::TargetFramework=net8.0]
/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/PostgresDistributedReaderWriterLock.cs(39,12): error RS0026: Symbol 'PostgresDistributedReaderWriterLock' violates the backcompat requirement: 'Do not add multiple overloads with optional parameters'. See 'https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md' for details. (https://github.com/dotnet/roslyn/blob/main/docs/Adding%20Optional%20Parameters%20in%20Public%20API.md) [/home/appveyor/projects/distributedlock/src/DistributedLock.Postgres/DistributedLock.Postgres.csproj::TargetFramework=net8.0]

These are not a new overload to any existing methods, I can't seems to get rid of it even after a rebase.

Please let me know how should I proceed with this, thanks!