Support DROP INDEX CONCURRENTLY - #3902
Open
eypcnckr wants to merge 1 commit into
Open
Conversation
eypcnckr
force-pushed
the
feat/drop-index-concurrently
branch
from
September 1, 2026 11:06
4b2bbcd to
2b190ed
Compare
roji
approved these changes
Sep 2, 2026
roji
left a comment
Member
There was a problem hiding this comment.
Yeah, makes sense for me to reuse IsCreateConcurrently for deletion as well rather than introduce yet another API point - the same reasons that lead to wanting an index to be created concurrently like lead to the same thing for drop.
CREATE INDEX CONCURRENTLY has been supported since npgsql#968, but the matching drop was not: Generate(DropIndexOperation) always emitted a plain DROP INDEX inside the migration transaction, so removing an index created with IsCreatedConcurrently() takes an ACCESS EXCLUSIVE lock on the table. Two things were missing: - NpgsqlMigrationsAnnotationProvider did not override ForRemove(ITableIndex), so the Npgsql:CreatedConcurrently annotation never reached the DropIndexOperation produced by the migrations differ. - Generate(DropIndexOperation) did not look at that annotation. Honour the annotation on both sides, and suppress the transaction for the drop, mirroring what Generate(CreateIndexOperation) already does.
roji
force-pushed
the
feat/drop-index-concurrently
branch
from
September 2, 2026 13:16
2b190ed to
2c5cb9f
Compare
roji
enabled auto-merge (squash)
September 2, 2026 13:17
Author
|
@roji hi, CI failed, but the only failure is build (windows-2022, 17, Release), and it's in the Start PostgreSQL 17 (Windows) step — the job exits after ~1.5 min, before the SDK setup and build steps run. All Linux jobs (PG 13–18, Release + Debug) passed, so the new tests do pass. Looks unrelated to the PR — could you re-run that one job when you get a chance? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #3901
What
CREATE INDEX CONCURRENTLYhas been supported since #968, but the matching dropwas not -
Generate(DropIndexOperation, ...)always emitted a plainDROP INDEXinside the migration transaction. Removing an index created with
IsCreatedConcurrently()therefore takes anACCESS EXCLUSIVElock on the table.Two things were missing, and both are needed for the feature to work end to end:
NpgsqlMigrationsAnnotationProviderdid not overrideForRemove(ITableIndex), soNpgsql:CreatedConcurrentlynever reached theDropIndexOperationproduced by thediffer - the operation had no annotations at all.
Generate(DropIndexOperation, ...)did not look at the annotation.This PR honours the annotation on both sides and suppresses the transaction for the
drop, mirroring
Generate(CreateIndexOperation, ...).Design note
I reused the existing
Npgsql:CreatedConcurrentlyannotation instead of introducinga new one, so
IsCreatedConcurrently()now means "created and droppedconcurrently". No new public API, and I think it matches the intent - but it is a
behaviour change for existing users: dropping such an index now runs outside the
migration transaction. If you would rather have an explicit
IsDroppedConcurrently(),say the word and I will rework it; the generator half stays the same either way.
Testing
DropIndexOperation_concurrently/DropIndexOperation_not_concurrentlyinNpgsqlMigrationsSqlGeneratorTestcover the generated SQL.Drop_index_concurrentlyinMigrationsNpgsqlTestcovers the full model-diffpath, mirroring the existing
Create_index_concurrently.What I could and could not run locally:
maintargetsnet11.0and pins the.NET 11 preview SDK in
global.json, which I do not have installed, so I could notbuild
mainor run its test suite. To verify the change I:hotfix/10.0.4worktree (identical code there,targets
net10.0), added the twoNpgsqlMigrationsSqlGeneratorTesttests and ranthem - both pass.
ForRemove(ITableIndex)half separately against the shipped 10.0.0package by registering an
IMigrationsAnnotationProviderwith exactly thisoverride and running the migrations differ: the
DropIndexOperationgoes fromhaving no annotations to carrying
Npgsql:CreatedConcurrently = True.So both halves are verified, but not together on
main- the newDrop_index_concurrentlyfunctional test in particular has not been executed. Pleaselet CI confirm, and I am happy to fix anything it turns up.