dotnet / efcore

EF Core is a modern object-database mapper for .NET. It supports LINQ queries, change tracking, updates, and schema migrations.
https://docs.microsoft.com/ef/
MIT License
13.63k stars 3.15k forks source link

Support context pool with multi tenancy - database per tenant #14625

Closed MaklaCof closed 1 year ago

MaklaCof commented 5 years ago

EF Core support context pooling since version 2.0.

But this approach is not (based on SO answer) possible with context factory, which I need for handling multi tenancy (database per tenant) approach.

It would be awesome if EF Core would support context pooling and solving problem with one database per tenant.

ajcvickers commented 5 years ago

@MaklaCof Can you give some details on why you are using context pooling? Do you have performance tests that show it makes a significant difference? If so, it would be great if you could share your numbers.

MaklaCof commented 5 years ago

@ajcvickers I was using it because it came out and promise better performance. It is always good to speed up parts of your code. I said to myself "I will gain performance, since I am using complex model, otherwise you would spend your resources building context pool.".

And I write this feature request because I am doing a lot of staff (reflection) in OnModelCreating. Like:

Unfortunately I don't have any performance tests.

ajcvickers commented 5 years ago

@MaklaCof I suspect that switching on or off context pooling in an application like you describe will have an negligible effect on performance. Context pooling can give a perf improvement, but typically it is only measurable in simple, repeated queries coming in at very high volume. So the recommendation here is to switch of pooling. If that turns out to result in a performance hit, then let us know and we'll reconsider.

MaklaCof commented 5 years ago

@ajcvickers I apologise to bother you, but doesn't context pooling also means connection pooling. After update from 2.1 to 2.2 I notice a lot of Debug messages from Microsoft.EntityFrameworkCore namespace.

Every query I run:

Surely this could be optimised. Opening and closing connection usually takes some time. So maybe not context pooling, but what about connection pooling? Is this duplicate to #13837?

I also noticed #11496 but I am confused. Are connection pooled or not?

roji commented 5 years ago

@MaklaCof connection pooling is implemented beneath the EF Core layer, in the ADO.NET layer: when you see "open connection" in your logs, this means EF Core is opening a DbConnection, but that pools as well. Assuming you're using SQL Server, then unless you specifically disable pooling by specifying Pooling=false in your connection string, it will be on by default. See these docs for more information.

ntziolis commented 2 years ago

@ajcvickers Sry to revive this issue. Please let me know if I should create a new one.

We have just run into this as well. Here is the scenario:

We could live without connection pooling, but we cannot live without being able to change the connection string on the fly (for each request). Please note that both the database changes as well as potentially the database server as well.

The only alternative we know as of now is to:

Maybe this could be solved with a similar approach as shard maps for elastic pools?

ajcvickers commented 2 years ago

@ntziolis EF Core comes with a DbContext factory that uses a pool. You can use it with D.I. using AddPooledDbContextFactory, or, in EF Core 6.0, use it without D.I..

ntziolis commented 2 years ago

@ajcvickers My bad for not making it super clear. The issue is that the PooledDbContextFactory does not switch the connection each time a context is requested:


                var connectionStringPlaceHolder = Configuration.GetConnectionString("DefaultConnection");
                services.AddPooledDbContextFactory<ApplicationDbContext>((serviceProvider, options) =>
                {
                    var tenantService = serviceProvider.GetRequiredService<ITenantService>();

                    var tenant = tenantService.GetCurrentTenant;
                    var connectionString = connectionStringPlaceHolder
                        .Replace("{dbServerName}", tenant.DatabaseServerName);
                        .Replace("{dbName}", tenant.DatabaseName);
                    options.UseSqlServer(connectionString);
                });

However I think we found a workaround solution that we think we can live with for the time being:

    public static class ApplicationDbContextFactoryExtensions
    {
        public static IServiceCollection AddApplicationDbContextFactory(this IServiceCollection services, Action<IServiceProvider, DbContextOptionsBuilder> optionsAction)
        {
            // we need to ensure all internal ef services are injected
            // efcore team recommends to not do this manually but instead use the AddDbContext extension method
            // in addition to the required services for ef it also adds the db context itself as a service
            // however we do not want the DbContext available as a service directly (this is why its removed after)
            // instead the we want the db context factory to create instances
            // this allows switching to pooling without rewriting the application
            services.AddDbContext<ApplicationDbContext>((serviceProvider, options) =>
            {
                throw new ApplicationException("ApplicationDbContext should not be directly injected. Please use DbContextFactory instead");
            });
            services.RemoveAll<ApplicationDbContext>();

            services.AddSingleton<IDbContextFactory<ApplicationDbContext>, ApplicationDbContextFactory>(
                sp => new ApplicationDbContextFactory(optionsAction, sp)
            );
            return services;
        }
    }

    public class ApplicationDbContextFactory : IDbContextFactory<ApplicationDbContext>
    {
        private readonly Action<IServiceProvider, DbContextOptionsBuilder> _optionsAction;
        private readonly IServiceProvider _serviceProvider;

        public ApplicationDbContextFactory(Action<IServiceProvider, DbContextOptionsBuilder> optionsAction, IServiceProvider serviceProvider)
        {
            _optionsAction = optionsAction;
            _serviceProvider = serviceProvider;
        }

        public ApplicationDbContext CreateDbContext()
        {
            var optionsBuilder = new DbContextOptionsBuilder<ApplicationDbContext>();
            _optionsAction(_serviceProvider, optionsBuilder);

            return new ApplicationDbContext(optionsBuilder.Options, _serviceProvider);
        }
    }

// In startup

                var connectionStringPlaceHolder = Configuration.GetConnectionString("DefaultConnection");
                services.AddApplicationDbContextFactory((serviceProvider, options) =>
                {
                    var tenantService = serviceProvider.GetRequiredService<ITenantService>();

                    var tenant = tenantService.GetCurrentTenant;
                    var connectionString = connectionStringPlaceHolder
                        .Replace("{dbServerName}", tenant.DatabaseServerName);
                        .Replace("{dbName}", tenant.DatabaseName);
                    options.UseSqlServer(connectionString);
                });

@ajcvickers Can you let us know your opinion if this is a workable approach or if there are any side-effects of AddDbContext we did not think about

ajcvickers commented 2 years ago

@ntziolis You can change the DbConnection used by DbContext, or just mutate the connection string on the existing connection, without creating a new DbContext instance. So you should be able to pull a context from the pool and then set the correct connection string.

ntziolis commented 2 years ago

Thank you for taking an interest in this. Its much appreciated. We have considered that as well, 2 reasons we have not done this so far:

I assuming since you suggested this approach that performance is better hence we will try this solution and report back here.

ajcvickers commented 2 years ago

@ntziolis EF typically opens the connection for each operation and closes it again immediately. This isn't related to context pooling, connection pooling, how long the DbContext instance lives, or what DbConnection object is used. This is because it's typically advantageous to have the connection open for as little time as possible, and connection opening/closing is very fast.

roji commented 2 years ago

to ensure the connection is reset every timer we would have todo it in a custom IDbContextFactory that derives from the pooled DbContextFactory and we didn't know how complex this would get

This is indeed a good way to implement multi-tenancy with context pooling, and really shouldn't be that complicated. It could just be a scoped service (so it can access the tenant ID) which wraps a singleton PooledDbContextFactory, and sets the connection string before handing out the context.

ntziolis commented 2 years ago

Thank you again guys for the feedback. This approach (closing connection reassign connection string) seems to work great.

Please note:

The adding and removing could be avoided if we c&p core registration or if the core registration code was not marked private.

Given the versatility of this approach (anything can be done to the context), would you consider something like as an addition to the ef source directly?

public static class CustomEntityFrameworkServiceCollectionExtensions
    {
        public static IServiceCollection AddPooledDbContextFactory<TContext>(
            this IServiceCollection serviceCollection,
                Action<IServiceProvider, DbContextOptionsBuilder> optionsAction,
                Action<IServiceProvider, TContext> configureContext,
                int poolSize = 1024
        )
            where TContext : DbContext
        {
            serviceCollection.AddPooledDbContextFactory<TContext>(optionsAction, poolSize);

            serviceCollection.RemoveAll<IDbContextFactory<TContext>>();

            serviceCollection.AddSingleton<IDbContextFactory<TContext>>(
                sp =>
                {
                    return new ConfigurableContextPooledDbContextFactory<TContext>(
                        sp, 
                        configureContext, 
                        sp.GetRequiredService<IDbContextPool<TContext>>()
                    );
                }
            );
            return serviceCollection;
        }
    }

    public class ConfigurableContextPooledDbContextFactory<TContext> : PooledDbContextFactory<TContext>
        where TContext : DbContext
    {
        private readonly IServiceProvider _serviceProvider;
        private readonly Action<IServiceProvider, TContext> _configureContext;

        public ConfigurableContextPooledDbContextFactory(IServiceProvider serviceProvider, Action<IServiceProvider, TContext> configureContext, IDbContextPool<TContext> pool) : base(pool)
        {
            _serviceProvider = serviceProvider;
            _configureContext = configureContext;
        }

        public override TContext CreateDbContext()
        {
            var context = base.CreateDbContext();

            _configureContext(_serviceProvider, context);

            return context;

        }

        public async override Task<TContext> CreateDbContextAsync(CancellationToken cancellationToken = default)
        {
            var context = await base.CreateDbContextAsync(cancellationToken);

            _configureContext(_serviceProvider, context);

            return context;
        }
    }

// Startup registration now looks like this

                var connectionStringPlaceHolder = Configuration.GetConnectionString("DefaultConnection");
                services.AddPooledDbContextFactory<ApplicationDbContext>(
                    (serviceProvider, options) =>
                    {                        
                        options.UseSqlServer(connectionStringPlaceHolder);
                    },
                    (sp, context) =>
                    {
                        var tenantService = sp.GetRequiredService<ITenantService>();

                        var tenant = tenantService.GetCurrentTenant()
                        var connectionString = connectionStringPlaceHolder.Replace("{dbName}", tenant.DatabaseName);

                        context.Database.CloseConnection();
                        context.Database.SetConnectionString(connectionString);
                    });
Eneuman commented 2 years ago

Can this be done using a DbConnectionInterceptor with? :

    public override InterceptionResult ConnectionOpening(DbConnection connection, ConnectionEventData eventData, InterceptionResult result)
    {
      var sqlConnection = (SqlConnection)connection;
      sqlConnection.ConnectionString = "<my connection striung>";
      return base.ConnectionOpening(connection, eventData, result);
    }

Or is this not beeing called when using Context pooling?

roji commented 2 years ago

@Eneuman context pooling doesn't affect interceptor usage in any way. The context may be pooled, but it still opens and closes connections when database operations need to be performed, just like unpooled contexts. So what you propose should work as well.

ntziolis commented 2 years ago

This looks very promising.

@roji Just be 100%: Does this mean we can be sure that the connection is always closed when create context returns the context?

Im asking as I'm not 100% but i think we had observed situations where the connection was already open right after the create context call.

Since the connection string would only be changed on connection open, a closed connection after the create context call would be required for this to work.

Eneuman commented 2 years ago

I did a quick test and it seems to be working:

public class OverrideConnectionStringDbConnectionInterceptor : DbConnectionInterceptor
  {
    private readonly Func<string> _getConnectionString;

    public OverrideConnectionStringDbConnectionInterceptor(Func<string> getConnectionString)
    {
      _getConnectionString = getConnectionString ?? throw new ArgumentNullException(nameof(getConnectionString));
    }

    public override InterceptionResult ConnectionOpening(DbConnection connection, ConnectionEventData eventData, InterceptionResult result)
    {
      var sqlConnection = (SqlConnection)connection;
      sqlConnection.ConnectionString = _getConnectionString();

      return base.ConnectionOpening(connection, eventData, result);
    }

    public override async ValueTask<InterceptionResult> ConnectionOpeningAsync(DbConnection connection, ConnectionEventData eventData, InterceptionResult result, CancellationToken cancellationToken = default)
    {
      var sqlConnection = (SqlConnection)connection;
      sqlConnection.ConnectionString = _getConnectionString();

      return await base.ConnectionOpeningAsync(connection, eventData, result, cancellationToken);
    }
  }

  public static class MySqlServerDbContextOptionsExtensions
  {
    public static DbContextOptionsBuilder WithConnectionString([NotNull] this DbContextOptionsBuilder optionsBuilder, IServiceProvider provider, Func<string> getSqlConnectionString)
    {
      var interceptor = new OverrideConnectionStringDbConnectionInterceptor(getSqlConnectionString);
      optionsBuilder.AddInterceptors(interceptor);
      return optionsBuilder;
    }
  }

Startup.cs

      services.AddDbContextPool<TContext>((provider, options) =>
      {
        options
        .UseSqlServer("Data Source=(local);Initial Catalog=Noname;", providerOptions => providerOptions.EnableRetryOnFailure())
        .WithConnectionString(() =>
        {
          var httpContext = provider.GetRequiredService<IHttpContextAccessor>().HttpContext;
          var tenantId = httpContext?.User.Claims.First(c => c.Type == "tenantId").Value;

          if (tenantId != null)
          {
            var configuration = provider.GetRequiredService<IConfiguration>();
            var connectionstring = configuration[$"Tenant{tenantId}:DatabaseConnectionString"];
            return connectionstring;
          }

          return "";
        });
      });

@roji, @ntziolis Please comment on any issues you find or if there is something that can be improved :) I have been looking for this for a while aswell.

roji commented 2 years ago

@ntziolis

Im asking as I'm not 100% but i think we had observed situations where the connection was already open right after the create context call.

If so, that would likely be a bug. Can you please try to reproduce this and open a new issue with the details?

ntziolis commented 2 years ago

@roji will do, but it could have very well been a false positive given that we have tried a lot of different approaches in a short period of time.

But from you comment I take it that connection can expected to be closed when getting a context from the pool that is good to have clarified, thank you.

@Eneuman we are currently in the middle of a launch but I'd like to switch our custom pool logic over to your connection interceptor right after and we'll report back to here.

oysandvik94 commented 2 years ago

I have the exact same challenge using HotChocolate, you guys are legends.

@Eneuman solution seems to work well. Looks like the IServiceProvider provider parameter in WithConnectionString extension method shouldnt be there though.

ViRuSTriNiTy commented 2 years ago

Either I am doing something wrong here or something has changed since @Eneuman posted the solution because on my side it is not working. I think I have tracked this down to the connection level, whereas the connection is just not closed when a pooled context is disposed (might also be the case for normal contexts, did not check that). This means that resolving the same context type again but for a different tenant will reuse this connection and the interceptor will not be called. The context then queries the wrong DB because the connection still points to the old tenant.

The comment of @ntziolis mentions that he wanted to report back whether the interceptor solution works for him. Since there is no report yet I assume it is not completely confirmed that it should definitely work.

Any tips on how to force closing the connection? Is this even a good idea? I remember having read something like "opening / closing connections is costly". So in the end I would have DBContext Pooling implemented but the connection stuff would be slowed down.

ntziolis commented 2 years ago

@ViRuSTriNiTy We have observed this as well (connection being open after retrieving context from the pool).

However we are not able to identify in which situations it occurs. So to be 100% sure the correct connection string is used we close the connection prior to resetting the connection string each time.

Since @ajcvickers highlighted that opening/closing connections is relatively cheap and is the default behavior when not using pooling for any operation with dbconext the db. We feel this is an acceptable solution.

Obviously it would be better if a connection would never be open when retrieved from the pool but since we can't pinpoint the issue to assist in finding this bug we have to live with the above.

This has been deployed to production in a med sized (millions of rows per tenant) multi tenant saas app without any issues or noticeable performance penalties so far.

Hope this helps.

ViRuSTriNiTy commented 2 years ago

@ntziolis Ahh great to have that confirmation. Would you mind sharing the code for how to close and reset of the connection?

ntziolis commented 2 years ago

I should have mentioned. We have not yet switched to the interceptor method but are still using what I posted prior to the interceptor solution was posted.

https://github.com/dotnet/efcore/issues/14625#issuecomment-1006471783

It might be possible to close the connection via the interceptor approach as well I just don't know. Since I haven't had time to apply this method to our application.

@Eneuman any thoughts?

ViRuSTriNiTy commented 2 years ago

@ntziolis Thanks a bunch, will dig through all the comments now. My bad, I did not read the full thread before putting a comment here ^^

ntziolis commented 2 years ago

No worries, it's a long thread ;)

roji commented 2 years ago

For those looking into this, there's now a documentation section specifically on context pooling and multi-tenant scenarios.

ViRuSTriNiTy commented 2 years ago

@roji Yes, I feel like having read this section a thousand times now. The example given there only works because it is relying on a single DB with multiple tenants which is a fact that is not so obvious at first.

Eneuman commented 2 years ago

@roji It would be great if that section was divided into a “Single db” and a “One db per tentant” section.

Has anything changed in latest EF regarding to open/closing connection?

roji commented 2 years ago

@ViRuSTriNiTy @Eneuman that section is about managing state in the context which changes (e.g. the tenant ID); that part is relevant regardless of whether you use a single DB or multiple. It indeed does not go into the mechanics of modifying the connection string based on the tenant ID, since that has to be done regardless of context pooling; that topic is covered in the multi-tenancy part of the docs.

But I agree we could improve our docs in this area...

ViRuSTriNiTy commented 2 years ago

Phewww, it works! After reading https://github.com/dotnet/efcore/issues/14625#issuecomment-1006421734 I updated my factory with the required connection string handling like

internal class MyDbContextFactory : IMyDbContextFactory
{
    private readonly IDbContextFactory<MyDbContext> _pooledDbContextFactory;
    private readonly ISqlConnectionStringProvider<MyDatabaseOptions> _sqlConnectionStringProvider;

    public MyDbContextFactory (IDbContextFactory<MyDbContext> pooledDbContextFactory,
        ISqlConnectionStringProvider<MyDatabaseOptions> sqlConnectionStringProvider)
    {
        _pooledDbContextFactory = pooledDbContextFactory;
        _sqlConnectionStringProvider = sqlConnectionStringProvider;
    }

    public MyDbContext CreateDynamicDbContext(Guid tenantId)
    {
        var dbContext = _pooledDbContextFactory.CreateDbContext();

        dbContext.TenantId = tenantId;

        var currentConnectionString = dbContext.Database.GetConnectionString();
        var newConnectionString = _sqlConnectionStringProvider.GetSqlConnectionString(licenseOwnerId);
        if (newConnectionString != currentConnectionString)
        {
            // make sure a possibly open connection is closed first as SetConnectionString() is documented
            // with "It may not be possible to change the connection string if existing connection, if any, is open."
            dbContext.Database.CloseConnection();

            dbContext.Database.SetConnectionString(newConnectionString);
        }

        return dbContext;
    }
}

and it now updates the connection correctly when doing something like

var dbContextFactory = services.GetRequiredService<IMyDbContextFactory>();

foreach (var tenantId in tenantIds)
{
    using (var dbContext = dbContextFactory.CreateDynamicDbContext(tenantId))
    {
        ...
    }
}

initially causing ObjectDisposedExceptions reported in #28533.

So the key parts not so obvious at first are

Fingers crossed that the connection string mangling does not cause side effects.

Btw: the interceptor approach was not in vain because it makes something like attaching a fresh access token to the connection possible.

ntziolis commented 2 years ago

Glad I could help.

Your summary is 100% how we do it.

The reason we had to do complicated mangling in regards to service registration in addition to what you described: We are using hot chocolate which retrieves the pooled db context factory itself. Hence no way to force it to use ours.

The solution was to replace the service that is retrieved when requesting the standard pooled db context factory interface. Which lead to the little involved service registration code you can see in my comment.

ViRuSTriNiTy commented 2 years ago

@ntziolis Yes, I once had a scenario like yours but not in the DB context pooling area. Thanks for the response.

kumarveerappan commented 2 years ago

Phewww, it works! After reading #14625 (comment) I updated my factory with the required connection string handling like

internal class MyDbContextFactory : IMyDbContextFactory
{
    private readonly IDbContextFactory<MyDbContext> _pooledDbContextFactory;
    private readonly ISqlConnectionStringProvider<MyDatabaseOptions> _sqlConnectionStringProvider;

    public MyDbContextFactory (IDbContextFactory<MyDbContext> pooledDbContextFactory,
        ISqlConnectionStringProvider<MyDatabaseOptions> sqlConnectionStringProvider)
    {
        _pooledDbContextFactory = pooledDbContextFactory;
        _sqlConnectionStringProvider = sqlConnectionStringProvider;
    }

    public MyDbContext CreateDynamicDbContext(Guid tenantId)
    {
        var dbContext = _pooledDbContextFactory.CreateDbContext();

        dbContext.TenantId = tenantId;

        var currentConnectionString = dbContext.Database.GetConnectionString();
        var newConnectionString = _sqlConnectionStringProvider.GetSqlConnectionString(licenseOwnerId);
        if (newConnectionString != currentConnectionString)
        {
            // make sure a possibly open connection is closed first as SetConnectionString() is documented
            // with "It may not be possible to change the connection string if existing connection, if any, is open."
            dbContext.Database.CloseConnection();

            dbContext.Database.SetConnectionString(newConnectionString);
        }

        return dbContext;
    }
}

and it now updates the connection correctly when doing something like

var dbContextFactory = services.GetRequiredService<IMyDbContextFactory>();

foreach (var tenantId in tenantIds)
{
    using (var dbContext = dbContextFactory.CreateDynamicDbContext(tenantId))
    {
        ...
    }
}

initially causing ObjectDisposedExceptions reported in #28533.

So the key parts not so obvious at first are

  • wrap the pooled DbContextFactory
  • create a DBContext from it
  • attach state like tenant ID and modify the connection string
  • hand out DBContext

Fingers crossed that the connection string mangling does not cause side effects.

Btw: the interceptor approach was not in vain because it makes something like attaching a fresh access token to the connection possible.

Hi, Can you please share sample file of startup.cs and and DBContextpool class ?

Eneuman commented 2 years ago

@roji Any idea why the interceptor approach isn't working as intended?

roji commented 2 years ago

@Eneuman which code are you referring to exactly?

Eneuman commented 2 years ago

@roji This one

https://github.com/dotnet/efcore/issues/14625#issuecomment-1025201624

As I understand it, connections are separated from the pooled dbContext, so changing the connection string in a interceptor should work fine, but it seem like the pooled dbContext sometimes already have a open connection attached to it (see above comments)

Is changing the connection string in a interceptor safe?

roji commented 2 years ago

@Eneuman EF allows explicitly configuring the context by providing an open DbConnection to e.g. UseSqlServer, in which case it's your responsibility to ensure it's connected to the right database beforehand. Barring that, connections should generally be closed, and so a ConnectionOpening interceptor should be fine. If you encounter an example to the contrary, please let us know by opening a new issue.

ViRuSTriNiTy commented 2 years ago

@kumarveerappan

Hi, Can you please share sample file of startup.cs ...

Startup.cs is nothing special, mostly the same code as documented by MS:

services.AddPooledDbContextFactory<MyDbContext>((provider, options) =>
{
    var dbOptions = provider.GetRequiredService<IOptions<DatabaseOptions>>().Value;

    options.UseSqlServer($"Data Source={dbOptions.ServerName};", sqlServerOptions =>
    {
        sqlServerOptions.CommandTimeout(120);
        sqlServerOptions.EnableRetryOnFailure();
    });

    // Add interceptor that attaches an access token to SqlConnection. This is required
    // because with DB Context Pooling this callback will only be called once when the
    // DbContext is created. So attaching an acces token here would sooner or later result
    // in an expired access token which in return causes a failing connection.
    var dbConnectionInterceptor = provider.GetRequiredService<IMyDbConnectionInterceptor>();

    options.AddInterceptors(dbConnectionInterceptor);
});

... and DBContextpool class ?

What do you mean by that? What is a DB context pool class?

kumarveerappan commented 2 years ago

@ViRuSTriNiTy , thanks a lot for your response.

I was mentioning about IMyDbConnectionInterceptor implementation. Is it possible to share the end to end sample implementation of DB per tenant approach.

mwasson74 commented 1 year ago

For the Hot Chocolate users in the crowd, I only implemented the interceptor as described by @Eneuman in their https://github.com/dotnet/efcore/issues/14625#issuecomment-1025201624. I then just add the interceptor to the DbContextOptionsBuilder on startup.

Something that tripped me up: I thought the interceptor was not working at first because when I checked the context at a breakpoint in my Hot Chocolate resolver, it had the wrong connection string. But since that resolver was only returning an IQueryable, the connection isn't actually opened until something (I assume Hot Chocolate) executes the IQueryable to return the results in the HTTP Response. So when that execution does happen, the interceptor's breakpoint hits, sets the connection string to what it should be, and then executes the IQueryable against the database using the correct connection string. Hopefully this helps someone else.

eskimoepos commented 1 year ago

@ViRuSTriNiTy , thanks a lot for your response.

I was mentioning about IMyDbConnectionInterceptor implementation. Is it possible to share the end to end sample implementation of DB per tenant approach.

A link to a working repo would be really useful if anyone has one - especially for noobs like me coming from .Net Framework. (Ideally, a HotChocolate Multi-Tennant app that supports a different DB per tenant). It's a little difficult trying to piece together the code snippets above.

aloksharma1 commented 1 year ago

@ViRuSTriNiTy , thanks a lot for your response. I was mentioning about IMyDbConnectionInterceptor implementation. Is it possible to share the end to end sample implementation of DB per tenant approach.

A link to a working repo would be really useful if anyone has one - especially for noobs like me coming from .Net Framework. (Ideally, a HotChocolate Multi-Tennant app that supports a different DB per tenant). It's a little difficult trying to piece together the code snippets above.

maybe you can use a readymade library like https://github.com/Finbuckle/Finbuckle.MultiTenant let me know if this does what you want to do?

eskimoepos commented 1 year ago

@ViRuSTriNiTy , thanks a lot for your response. I was mentioning about IMyDbConnectionInterceptor implementation. Is it possible to share the end to end sample implementation of DB per tenant approach.

A link to a working repo would be really useful if anyone has one - especially for noobs like me coming from .Net Framework. (Ideally, a HotChocolate Multi-Tennant app that supports a different DB per tenant). It's a little difficult trying to piece together the code snippets above.

maybe you can use a readymade library like https://github.com/Finbuckle/Finbuckle.MultiTenant let me know if this does what you want to do?

Thanks for the suggestion of Finbuckle. I would have definitely used that I think, although I wrote my own middleware in the end. Thanks for the reply.

adnan-kamili commented 2 months ago

This thread has been incredibly insightful. We recently transitioned from a shared database to a tenant-per-database architecture. In doing so, we realized we lost the benefits of DbContext pooling and need to implement a solution as discussed here. Initially, I had confused DbContext pooling with connection pooling, assuming they were the same. Here’s what I’ve finally concluded:

  1. DbContext Pooling: DbContext pooling in EF Core only pools the DbContext objects. It reuses instances of DbContext to improve performance and reduce the overhead of creating new DbContext instances for each request.

  2. Connection Pooling: Connection pooling is handled at a lower level, typically by the database provider (in our case, Npgsql for PostgreSQL). By setting Pooling=true in our connection string, we can enable connection pooling, which allows the provider to reuse existing database connections rather than creating new ones.

  3. Interaction Between DbContext Pooling and Connection Pooling: DbContext pooling and connection pooling are independent. We can have connection pooling without DbContext pooling and vice versa. When a DbContext is reused from the pool, it will use a connection from the connection pool (if connection pooling is enabled).

  4. Tenant per Database Architecture: In a tenant-per-database architecture, each tenant has its own database, which leads to fragmented connection pools. This is because each tenant's DbContext instance will connect to a different database, resulting in separate connection pools for each tenant. This fragmentation can impact the efficiency of connection pooling, as each pool will be smaller and might not benefit as much from the pooling.

@roji can you please confirm my understanding?

If the above is true, I don't find much value in adding back DBContext pooling, as at max it can slightly improve the performance at the application level (not the database end). It won't minimize the number of DB connections which is a major issue when moving to DB per Tenant architecture.

roji commented 2 months ago

@adnan-kamili yep, most of what you write is accurate.

If the above is true, I don't find much value in adding back DBContext pooling, as at max it can slightly improve the performance at the application level (not the database end). It won't minimize the number of DB connections which is a major issue when moving to DB per Tenant architecture.

Here I think you missed something. Since DbContext pooling and connection pooling are fully independent, a single pool of DbContexts can handle everything for your entire application (across all tenants), even when the lower-level connection pools are fragmented because of multiple databases. So DbContext pooling shouldn't be any less useful in multi-tenant scenarios than they are in other scenarios. Makes sense?

adnan-kamili commented 2 months ago

@roji Thanks for the confirmation.

I agree that using a single pool is better than instantiating a new DbContext for each request. However, we would still like to evaluate the performance impact for our specific scenario. Given this is such a common requirement, ef core should have provided some native solution to handle this.

After moving to a tenant-per-database model, we observed an immediate increase in memory usage. Previously, our memory usage ranged between 400-600 MB, even if the app ran for months without a restart. However, post-migration, the average memory usage gets up to 1.3 GB within a few hours (and stays there), sometimes reaching up to 1.5 GB. Since we’ve set a limit of 1.5 GB, Kubernetes restarts the pod when this limit is reached.

I'm uncertain about the cause of this increase in memory usage. Possible factors could include a large number of fragmented connection pools, an excessive number of DbContext instances, or the model cache (since each connection string is different even though the context (TenantDbContext) remains the same). We have around 1000 databases distributed on two Postgres Clusters (with 10-20K reqs/min). Could you provide any insights into a possible cause for this issue?

roji commented 2 months ago

I agree that using a single pool is better than instantiating a new DbContext for each request. However, we would still like to evaluate the performance impact for our specific scenario. Given this is such a common requirement, ef core should have provided some native solution to handle this.

Multi-tenancy is complex, and there are many ways to implement it (database-per-tenant, schema-per-tenant, other options) - so it's difficult to provide something in EF that "just works" (but I agree we should explore this). We do have these docs that should help with setting up DbContext pooling in multi-tenant scenarios.

I'm uncertain about the cause of this increase in memory usage. Possible factors could include a large number of fragmented connection pools, an excessive number of DbContext instances, or the model cache (since each connection string is different even though the context (TenantDbContext) remains the same).

Connection pool fragmentation generally doesn't produce significant client-side memory load, but rather load on the database because of the increased number of connections. Also, different connection strings in themselves doesn't cause the model to be rebuilt, should there shouldn't be much trouble there.

In general in this kind of situation, rather than speculating it's best to do a memory profiling session and see where the memory load is actually coming from; at that point the root cause can be investigated further.

adnan-kamili commented 2 months ago

@roji Thanks for your response. We will investigate it further.

sgsmittal commented 2 months ago

Phewww, it works! After reading #14625 (comment) I updated my factory with the required connection string handling like

internal class MyDbContextFactory : IMyDbContextFactory
{
    private readonly IDbContextFactory<MyDbContext> _pooledDbContextFactory;
    private readonly ISqlConnectionStringProvider<MyDatabaseOptions> _sqlConnectionStringProvider;

    public MyDbContextFactory (IDbContextFactory<MyDbContext> pooledDbContextFactory,
        ISqlConnectionStringProvider<MyDatabaseOptions> sqlConnectionStringProvider)
    {
        _pooledDbContextFactory = pooledDbContextFactory;
        _sqlConnectionStringProvider = sqlConnectionStringProvider;
    }

    public MyDbContext CreateDynamicDbContext(Guid tenantId)
    {
        var dbContext = _pooledDbContextFactory.CreateDbContext();

        dbContext.TenantId = tenantId;

        var currentConnectionString = dbContext.Database.GetConnectionString();
        var newConnectionString = _sqlConnectionStringProvider.GetSqlConnectionString(licenseOwnerId);
        if (newConnectionString != currentConnectionString)
        {
            // make sure a possibly open connection is closed first as SetConnectionString() is documented
            // with "It may not be possible to change the connection string if existing connection, if any, is open."
            dbContext.Database.CloseConnection();

            dbContext.Database.SetConnectionString(newConnectionString);
        }

        return dbContext;
    }
}

and it now updates the connection correctly when doing something like

var dbContextFactory = services.GetRequiredService<IMyDbContextFactory>();

foreach (var tenantId in tenantIds)
{
    using (var dbContext = dbContextFactory.CreateDynamicDbContext(tenantId))
    {
        ...
    }
}

initially causing ObjectDisposedExceptions reported in #28533. So the key parts not so obvious at first are

  • wrap the pooled DbContextFactory
  • create a DBContext from it
  • attach state like tenant ID and modify the connection string
  • hand out DBContext

Fingers crossed that the connection string mangling does not cause side effects. Btw: the interceptor approach was not in vain because it makes something like attaching a fresh access token to the connection possible.

Hi, Can you please share sample file of startup.cs and and DBContextpool class ?

Hi @kumarveerappan did you got the complete sample code ?