npgsql / efcore.pg

Entity Framework Core provider for PostgreSQL
PostgreSQL License
1.49k stars 215 forks source link

Is it possible to connect to multiple databases in a multitenant scenario? #3204

Closed rowanfreeman-acutro closed 2 days ago

rowanfreeman-acutro commented 2 weeks ago

I think my question is related to Make NpgsqlDataSource a scoped service instead of singleton.

I've search everywhere trying to figure out how to (properly) do multiple connections with EFCore and Npgsql.

My question is: Am I doing it right? Is there a better way?

I need to connect to multiple different databases with different connection strings. I do this with an ITenantContextAccessor scoped service which gets me the tenant connection string.

builder.Services.AddDbContext<ApplicationDbContext>(
    (provider, db) =>
    {
        var tenantAccessor = provider.GetRequiredService<ITenantContextAccessor>();

        var connectionString = tenantAccessor.Tenant.ConnectionString;
        var dataSourceBuilder = new NpgsqlDataSourceBuilder(connectionString);
        var connection = dataSourceBuilder.Build();

        db.UseNpgsql(connection);
    }

This solution works, but I get a warning from .NET

An 'IServiceProvider' was created for internal use by Entity Framework.
An additional 'IServiceProvider' was created for internal use by Entity Framework. An existing service provider was not used due to the following configuration changes: .

My understanding is that this isn't great, and could cause problems down the track. From what I understand, EntityFramework (or Npgsql?) doesn't like creating multiple data sources dynamically like that.

rowanfreeman-acutro commented 1 week ago

I've come up with a solution to this problem using an EF interceptor. But after spending so many hours researching this, I'm really skeptical about my solution because I haven't seen it proposed anywhere else.

But it works; I successfully connected to four different databases, depending on the IServiceScope, using EF and Npgsql. No warnings, no errors.

public sealed class TenantDbConnectionInterceptor(ITenantContextAccessor tenantContextAccessor)
    : DbConnectionInterceptor
{
    public override ValueTask<InterceptionResult> ConnectionOpeningAsync(
        DbConnection connection,
        ConnectionEventData eventData,
        InterceptionResult result,
        CancellationToken cancellationToken = new()
    ) => ValueTask.FromResult(ConnectionOpening(connection, eventData, result));

    public override InterceptionResult ConnectionOpening(
        DbConnection connection,
        ConnectionEventData eventData,
        InterceptionResult result
    )
    {
        var tenant = tenantContextAccessor.Tenant;
        var connectionString = tenant?.ConnectionString ?? string.Empty;

        connection.ConnectionString = connectionString;

        return result;
    }
}

Can anyone see a problem with this?

roji commented 1 week ago

@rowanfreeman-acutro the main point to keep in mind, is that NpgsqlDataSources should be long lived - created once and used throughout the application. Your original code above creates an NpgsqlDataSource for each and every DbContext, which is bad.

We definitely need to do some work to improve multi-tenant scenario (especially with NpgsqlDataSource) - I hope to get to do work on this for 9.0. But in the meantime, take a look at this link, which discusses DbContext pooling and multi-tenancy.

In a nutshell, you generally need to have a singleton registry of NpgsqlDataSources - one per tenant; this registry is a singleton since as I wrote above, a data source instance should live throughout your application. Then, you need to have a way for your DbContext to get initialized with the correct NpgsqlDataSource instance - the link above shows how to do that for a DbContextFactory.

Note that currently, having different NpgsqlDataSource instances (one-per-tenant) causes some trouble inside EF itself - that's exactly what #3086 will fix for 9.0.

rowanfreeman-acutro commented 2 days ago

Thanks @roji, great stuff. Yeah that's the path I originally went down. I had a singleton class that had a dictionary of a few NpgsqlDataSources - one for each tenant, so that they would be reused. I also tried using a static class for the same thing.

But no matter what I tried, including consulting the link you mentioned, I just couldn't get anything to work.

Eventually EF creates too many internal IServiceProviders and throws an exception/warning.

Thanks though, it sounds like 3086 will contribute to this becoming more viable.

roji commented 1 day ago

I had a singleton class that had a dictionary of a few NpgsqlDataSources - one for each tenant, so that they would be reused.

Yep, that's a very good approach.

Eventually EF creates too many internal IServiceProviders and throws an exception/warning.

That warning just means that you're instantiating more than some fix number of service providers, to help users catch the scenario where they're creating a service provider for each and every DbContext (which would be very bad). If you have a fixed number of tenants, and each tenant has a single NpgsqlDataSource (which, before #3086, causes a single EF service provider to be created), then you can safely suppress the warning and just proceed.