Description
IsCreatedConcurrently() makes migrations emit CREATE INDEX CONCURRENTLY and
suppresses the surrounding transaction (#967, #968, #1214). There is no equivalent
for dropping: Generate(DropIndexOperation, ...) always emits a plain
DROP INDEX "IX_People_Age";
inside the migration transaction. Dropping an index that way takes an
ACCESS EXCLUSIVE lock on the table, which is exactly what someone using
IsCreatedConcurrently() was trying to avoid in the first place. PostgreSQL has
supported DROP INDEX CONCURRENTLY since 9.2, and like the create form it cannot
run inside a transaction block.
There are two separate gaps:
1. The annotation never reaches the operation. NpgsqlAnnotationProvider.For(ITableIndex, designTime)
yields Npgsql:CreatedConcurrently, but NpgsqlMigrationsAnnotationProvider does
not override ForRemove(ITableIndex), so nothing is attached to the
DropIndexOperation the differ produces.
2. The generator ignores it. Even when the annotation is present (for example
set by hand in a migration), Generate(DropIndexOperation, ...) never looks at it.
Reproduction
Diffing a model that has HasIndex("Age").IsCreatedConcurrently() against one that
does not, with 10.0.0:
=== DROP direction (index removed from model) ===
DropIndexOperation
SQL: DROP INDEX "IX_People_Age";
(suppressTransaction = False)
=== CREATE direction (index added to model) ===
CreateIndexOperation
annotation: Npgsql:CreatedConcurrently = True
SQL: CREATE INDEX CONCURRENTLY "IX_People_Age" ON "People" ("Age");
(suppressTransaction = True)
Note the drop operation carries no annotations at all.
Adding a ForRemove(ITableIndex) override that yields the annotation is enough to
make it appear on the DropIndexOperation, confirming the hook is the right one:
=== DROP direction (index removed from model) ===
DropIndexOperation
annotation: Npgsql:CreatedConcurrently = True
Suggested fix
Override ForRemove(ITableIndex) in NpgsqlMigrationsAnnotationProvider, and
honour the annotation in Generate(DropIndexOperation, ...) with
EndStatement(builder, suppressTransaction: concurrently), mirroring
Generate(CreateIndexOperation, ...).
Design question
I reused the existing Npgsql:CreatedConcurrently annotation rather than adding a
new one, so IsCreatedConcurrently() means "this index is created and dropped
concurrently". That needs no new public API and is what I would expect as a user,
but it does change behaviour for existing code: dropping such an index would now
run outside the migration transaction. A separate IsDroppedConcurrently() would
be more explicit, at the cost of another API (and it would still have to be
declared while the index exists, so it can reach the snapshot).
Happy to switch to whichever you prefer - PR attached implements the first option.
Description
IsCreatedConcurrently()makes migrations emitCREATE INDEX CONCURRENTLYandsuppresses the surrounding transaction (#967, #968, #1214). There is no equivalent
for dropping:
Generate(DropIndexOperation, ...)always emits a plaininside the migration transaction. Dropping an index that way takes an
ACCESS EXCLUSIVElock on the table, which is exactly what someone usingIsCreatedConcurrently()was trying to avoid in the first place. PostgreSQL hassupported
DROP INDEX CONCURRENTLYsince 9.2, and like the create form it cannotrun inside a transaction block.
There are two separate gaps:
1. The annotation never reaches the operation.
NpgsqlAnnotationProvider.For(ITableIndex, designTime)yields
Npgsql:CreatedConcurrently, butNpgsqlMigrationsAnnotationProviderdoesnot override
ForRemove(ITableIndex), so nothing is attached to theDropIndexOperationthe differ produces.2. The generator ignores it. Even when the annotation is present (for example
set by hand in a migration),
Generate(DropIndexOperation, ...)never looks at it.Reproduction
Diffing a model that has
HasIndex("Age").IsCreatedConcurrently()against one thatdoes not, with 10.0.0:
Note the drop operation carries no annotations at all.
Adding a
ForRemove(ITableIndex)override that yields the annotation is enough tomake it appear on the
DropIndexOperation, confirming the hook is the right one:Suggested fix
Override
ForRemove(ITableIndex)inNpgsqlMigrationsAnnotationProvider, andhonour the annotation in
Generate(DropIndexOperation, ...)withEndStatement(builder, suppressTransaction: concurrently), mirroringGenerate(CreateIndexOperation, ...).Design question
I reused the existing
Npgsql:CreatedConcurrentlyannotation rather than adding anew one, so
IsCreatedConcurrently()means "this index is created and droppedconcurrently". That needs no new public API and is what I would expect as a user,
but it does change behaviour for existing code: dropping such an index would now
run outside the migration transaction. A separate
IsDroppedConcurrently()wouldbe more explicit, at the cost of another API (and it would still have to be
declared while the index exists, so it can reach the snapshot).
Happy to switch to whichever you prefer - PR attached implements the first option.