npgsql / efcore.pg

Entity Framework Core provider for PostgreSQL
PostgreSQL License
1.58k stars 227 forks source link

Support for manual application of indexes when using code first #914

Closed williamdenton closed 3 years ago

williamdenton commented 5 years ago

Using code first in Efcore means defining indexes as well as schema in code, generated migrations then apply migrations to the database.

For large tables, adding a new index must be done with care to avoid locking to keep the db running while the migration is taking place.

Currently our work around is to

  1. create index using code first
  2. create migration
  3. apply migration manually to production db and sql update the __migration_history

this means that on the next migration execution (which we do as part of the deploy) there is no work to do.

A couple of thinking points:

  1. It would be very helpful to have a create index if not exists option, so indexes could be applied manually, then added back to the model
  2. supporting the concurrent option is useful but not as useful as the not exists check before creation option
  3. bumping the db version as the index is manually created is not ideal as this happens outside of a normal deployment process, and if the service restarts for any reason, the expected db versions wont match and the app wont restart (thats a failsafe we have to ensure we don't run on unexpected schema versions to avoid data corruption)
roji commented 5 years ago

@williamdenton thanks for raising this important question, it's something I thought out before but never had time to fully explore.

First, when you say "apply migration manually to production db", do you mean issue the CREATE INDEX yourself via SQL, and then updating __MigrationHistory yourself? If so, what exactly do you gain by doing this, as opposed to just executing the EF migration as usual? Is this simply a way to add CONCURRENTLY to the creation?

If that is so, then adding an option to have EF Core generated CONCURRENTLY on index creation seems like the right solution. In fact, I'm wondering whether it should not become the default - there are some caveats to concurrent index creation (here are the docs) but the pros seem to outweigh the cons. Either way we would still allow users to change the behavior via the fluent API.

Otherwise I may be missing something in your description.

FYI the generally recommended way to apply migrations to production databases is to generate an SQL migration script and apply that to databases.

williamdenton commented 5 years ago

One step of our deployment process is to execute migrations using database.Migrate(), once this is complete then the new service is deployed and the previous version drained. For schema changes we are careful to always perform widening migrations that apply quickly that are compatible with the prior version still running and the new version pending to be deployed. (ie adding a not-null column is not possible, the column must be added, backfilled, then updated to be not null across at least three separate deployments).

Index creation sometimes happens as a result of performance tuning. Index creation on very large tables can take some time to complete and we sometimes choose to do this manually via psql rather than normal deployment.

What i would like is an option where ef can apply the index in dev, staging and newly bootstrapped environments automatically, but for some environments we choose to apply the index in advance, perhaps in response to a production issue we face as the database volume grows over time and the query optimizer changes the plan as statistics are updated.

Wrapping the create index in an if block that checks if the index name already exists would be helpful to enable this scenario. so we dont have to mess the the migrations table is is fraught with human error.

Im not sure of how this would be exposed in the api and i am not suggesting that this become the default behaviour of creating indexes.


the request i am making here is orthogonal from the concurrent option, although that would improve the options available to us in creating indexes on large tables.

williamdenton commented 5 years ago

guessing any change would probably be made in here? (this is from a 5 min search of github rather than any deep understanding of what is actually happening

https://github.com/npgsql/Npgsql.EntityFrameworkCore.PostgreSQL/blob/7b0eb5f0a85401953e6c2fe2abc40276777970a5/src/EFCore.PG/Migrations/NpgsqlMigrationsSqlGenerator.cs#L556-L567

roji commented 5 years ago

Thanks for the added details.

First, it would indeed be trivial to add IF NOT EXISTS to CREATE INDEX. This is generally not done, probably because it is not needed if EF manages your schema, and mixed EF/manual schema seem very error-prone. For example, a migration could be thought to have been applied, but in fact table creation was skipped because someone already created a slightly different table manually. It seems like a dangerous-enough pit of failure to not have it. That's for the general case - I do agree that chances for this kind of mess are less for indices.

But I'm still curious about the way you work: EF allows you to generate an SQL script for your migrations, and apply it outside of your program (this is in fact the recommended practice). This would do all schema changes and update the migration history table, without risk of human error. What advantage does manual index creation have over this?

BTW any opinion on whether CONCURRENTLY should be the default or not?

@ajcvickers @divega any opinion here?

williamdenton commented 5 years ago

I'll give you a background on how we deploy, it's always helpful to understand how others do it, so i'll share mine.

Infrastructure wise I run in AWS using docker on ECS using Amazon RDS Aurora.

My app consists of several web nodes behind an ALB (application load balancer), typically 3 (across three availability zones). Also a job engine of which I run a single instance.

Deployments look a little like:

  1. Merge to master
  2. build artifacts in CI system (either concourse, or jenkins depending on microservice)
  3. Octopus Deploy is responsible for orchestrating deployments
  4. execute ecs task with docker image which runs the migrations against the database. essentially just runs database.Migrate(). Typically there are no migrations to deploy. The task exists with a success exit code if either there are no migrations or the database is up to date.
  5. Deploy the api, this is done by adding 3 new instances to the load balancer, draining the old api instances (letting requests finish, but not serving new ones) then stopping the old containers. ECS manages this process.
  6. deploy the job engine, this stops the old job engine before starting the new one (unlike how the api is done)
  7. the deployment is now complete.

This is done for each environment. It happens automatically to the staging/test environment after the build completes. Then to production by clicking deploy in octopus by the developer who write the PR that was initially merged. Developers are responsible for their code all the way to production (and beyond)

If a database migration was part of the merged code that will usually be the only code in the deployment, no functional changes to the service are deployed at the same time. This is to reduce risk.

Rinse - repeat. Process from merging to deployment to production can be as short as 10 minutes but typically about 30 - depending on risk and need to test new functionality in the test environment. We rely heavily on feature flagging to disable new functional code in production, but still allow that code path to be tested in the staging/test environment. Releasing of new functionality to production is simply changing the value of the feature flag to true for that environment. Once feature is on in all environments, the technical debt of the feature flag infrastructure is then paid down and the if statements removed.

Very rarely an operational issue is encountered that may need immediate remediation in production, this is where an index may be applied directly to the database then back ported into the ef model to keep schema consistent. We call this co-piloting as this is always done with two people, two sets of eyes on what is being applied. This is a fairly rare case, but in this situation it would be valuable to have the if not exists flag on the index name, so we have a trust me, i did this manually escape hatch. This may seem unnecessary given how fast we can deploy changes, but this is still a useful option to have.

Currently we also co-pilot new indexes to production because they cause locking of the tables and take some time to execute. Running the migrations in the usual manner above for an index that takes more than 10 -20 seconds to build would result in unacceptable table locking and essentially result in service downtime. The concurrent option would help here.

This use case would never apply to tables or other schema objects.

roji commented 5 years ago

Thanks for the detail @williamdenton, the process is very clear. But I'm still struggling to understand why for your manual updates you can't just use the EF-generated migration SQL script... It's essentially the same as the CREATE INDEX that you're already doing manually, except it also updates the migrations history table for you, with no risk of error anywhere.

All this is regardless of adding CONCURRENTLY to CREATE INDEX in migrations, which I'm leaning towards but no yet 100% sure (would be good to have other opinions).

roji commented 5 years ago

@williamdenton CREATE INDEX CONCURRENTLY is being added in #967.

Am going to move this discussion to the backlog as you haven't posted any responses on the other questions - but we can definitely revisit.

anber500 commented 3 years ago

I'm using EF 3.1.0.

Our DB Admin added an index to our production Postgres database, which I'm trying to add to our EF migrations. The SQL looks like this:

CREATE INDEX CONCURRENTLY ix_urlhistory_fkactionid_aspnetsessionid ON public.urlhistory USING btree (fk_actionid, aspnetsessionid) TABLESPACE pg_default;

My C# code looks as follows:

entity.HasIndex(e => new {e.FkActionId, e.AspnetsessionId }) .IsCreatedConcurrently(true) .HasName("ix_urlhistory_fkactionid_aspnetsessionid");

However, the SQL INDEX that's generated looks as follows:

CREATE INDEX ix_urlhistory_fkactionid_aspnetsessionid ON public.urlhistory USING btree (fk_actionid ASC NULLS LAST, aspnetsessionid COLLATE pg_catalog."default" ASC NULLS LAST) TABLESPACE pg_default;

Why is the INDEX CONCURRENTLY not added to the index? Am I missing something? It seems straight forward to use if I look at the code:

https://github.com/npgsql/efcore.pg/commit/2ee912f140496ca2a1d892b9c239dd551f5a174f#diff-da8c51818628dc3e0d19acf2a6431f0ae2e4422c5e985800e3976780deee56ee

roji commented 3 years ago

@anber500 I'm seeing the right SQL DDL being generated with CONCURRENTLY, but either way, please post a new issue (this issue isn't about CREATE INDEX CONCURRENTLY), and include a full code sample to reproduce the problem.