Fix: SuppressTransaction support for ConcurrentIndex correctness - #48
Merged
Merged
Conversation
ConcurrentIndex() on PostgreSQL requires execution outside a transaction block, but MigrationRunner wrapped all operations in a single transaction. - Add SuppressTransaction property to MigrationOperation - Add .SuppressTransaction() fluent modifier to MigrationBuilder - Add BuildPartitionedSql() to separate transactional/non-transactional ops - Auto-imply SuppressTransaction for PostgreSQL ConcurrentIndex operations - Split MigrationRunner.ApplyMigrationAsync into two phases: 1. Transactional phase (inside BeginTransactionAsync) 2. Non-transactional phase (direct connection, no transaction) - Reverse phase order for RollbackMigrationAsync (non-tx first, then tx) - Add warning log when migrations contain non-transactional operations Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lication - Fix execution_time_ms regression: record actual elapsed milliseconds in history row instead of hardcoded 0 - Eliminate triple SQL rendering: BuildPartitionedSql now returns a 3-tuple (txSql, nonTxSql, allSql), deriving combined SQL from partitioned results instead of calling BuildSql separately - Remove redundant else branch in ApplyMigrationAsync: unified into a single transaction block that always writes the history row - Add test: only-non-transactional migration still records history - Add test: Phase 2 failure leaves history row committed (partial-apply state) - Add test: execution_time_ms is recorded correctly Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
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
1 participant
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.
Summary
ConcurrentIndex()on PostgreSQL was broken —CREATE INDEX CONCURRENTLYcannot run inside a transaction block, butMigrationRunnerwrapped all operations in a singleDbTransaction. This PR adds aSuppressTransaction()fluent modifier and partitions runner execution into transactional and non-transactional phases, with PostgreSQL auto-suppression forConcurrentIndex().Reason for Change
MigrationRunner.ApplyMigrationAsync()wrapped the entire migration in a single transaction. PostgreSQL errors withCREATE INDEX CONCURRENTLY cannot run inside a transaction block, making the existingConcurrentIndex()modifier unusable on PostgreSQL. This is a correctness bug — the feature silently fails or errors.Impact
MigrationBuildergains a new.SuppressTransaction()fluent modifierMigrationRunnernow partitions operations into transactional (Phase 1) and non-transactional (Phase 2) phasesConcurrentIndex()operations are automatically suppressed from transactions — no user action neededConcurrentIndex()(WITH (ONLINE = ON)) is unaffected — it still runs inside transactions as beforePlan items implemented as specified
MigrationOperation: Addedbool SuppressTransactionpropertyMigrationBuilder: Added.SuppressTransaction()fluent modifierMigrationBuilder: AddedBuildPartitionedSql()returning(txSql, nonTxSql, allSql)for phase splittingMigrationRunner.ApplyMigrationAsync(): Split into transactional phase (Phase 1) then non-transactional phase (Phase 2)MigrationRunner.RollbackMigrationAsync(): Reverse order — non-transactional first, then transactionalConcurrentIndex()on PostgreSQL automatically impliesSuppressTransactionDeviations from plan implemented
Gaps in original plan implemented
BuildPartitionedSqlreturns a 3-tuple(txSql, nonTxSql, allSql)— combined SQL derived from partitioned results to avoid redundant rendering passes throughDdlRendererNonTransactionalWarning,NonTransactionalSqlGenerated) emitted when migrations contain suppressed operationsApplyMigrationAsync— single code path handles both mixed and non-tx-only migrationsMigration Steps
No migration needed — this is a runtime behavior change in
MigrationRunner.Performance Considerations
BuildPartitionedSqlderives combined SQL from partitioned results instead of callingDdlRendererseparatelySecurity Considerations
Breaking Changes
Consumer-facing
.SuppressTransaction()modifier is opt-in.Internal
MigrationBuilder.BuildPartitionedSql()returns a 3-tuple(string, string, string)(internal method)MigrationBuilder.HasNonTransactionalOperations()added (internal method)MigrationOperation.SuppressTransactionproperty added