Opened 3 months ago

Closed 3 months ago

Last modified 3 months ago

#33744 closed Cleanup/optimization (wontfix)

sqlmigrate wraps it's output in BEGIN/COMMIT even if atomic transactions are disabled

Reported by: Joe Meissler Owned by: nobody
Component: Migrations Version: 4.0
Severity: Normal Keywords:
Cc: Simon Charette Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

This is very similar to ticket:30189. With that fix, sqlmigrate now takes into consideration migration.atomic and connection.features.can_rollback_ddl. It does not, however, consider if the schema editor is using an atomic migration.

sqlmigrate can be updated to use migration.atomic and schema_editor.atomic_migration instead of migration.atomic and connection.features.can_rollback_ddl.

I'm happy to work on this once it is approved.

Change History (5)

comment:1 in reply to:  description ; Changed 3 months ago by Mariusz Felisiak

Cc: Simon Charette added
Resolution: needsinfo
Status: newclosed
Summary: sqlmigrate wraps it's outpout in BEGIN/COMMIT even if atomic transactions are disabledsqlmigrate wraps it's output in BEGIN/COMMIT even if atomic transactions are disabled
Type: BugCleanup/optimization

Replying to Joe Meissler:

It does not, however, consider if the schema editor is using an atomic migration.

sqlmigrate can be updated to use migration.atomic and schema_editor.atomic_migration instead of migration.atomic and connection.features.can_rollback_ddl.

Schema editor instance is created later in MigrationLoader (it isn't available in sqlmigrate's handle()) and its atomic value is based on migration.atomic. I'm not sure when these two flags might have different values. Can you provide an example? How you would like to introduce SchemaEditor in the sqlmigrate?

comment:2 in reply to:  1 ; Changed 3 months ago by Joe Meissler

Replying to Mariusz Felisiak:

Replying to Joe Meissler:

It does not, however, consider if the schema editor is using an atomic migration.

sqlmigrate can be updated to use migration.atomic and schema_editor.atomic_migration instead of migration.atomic and connection.features.can_rollback_ddl.

Schema editor instance is created later in MigrationLoader (it isn't available in sqlmigrate's handle()) and its atomic value is based on migration.atomic. I'm not sure when these two flags might have different values. Can you provide an example? How you would like to introduce SchemaEditor in the sqlmigrate?

We can access schema_editor via with connection.schema_editor() as editor, similar to what is used in the migrate command. As for the atomic flag, BaseSchemaEditor has the atomic_migration attribute. You are correct that in the loader, the schema_editor is instantiated with migration.atomic. If, however, the editor itself always sets atomic = False, it doesn't matter that the migration is atomic.

comment:3 in reply to:  2 ; Changed 3 months ago by Mariusz Felisiak

Replying to Joe Meissler:

We can access schema_editor via with connection.schema_editor() as editor, similar to what is used in the migrate command. As for the atomic flag, BaseSchemaEditor has the atomic_migration attribute. You are correct that in the loader, the schema_editor is instantiated with migration.atomic. If, however, the editor itself always sets atomic = False, it doesn't matter that the migration is atomic.

So you have a 3rd party backend that ignores migration.atomic and always set SchemaEditor.atomic_migration = False and in the same time can roll back DDL (can_rollback_ddl=True), that's unusual. I'm not sure it's worth extra code 🤔.

comment:4 in reply to:  3 Changed 3 months ago by Joe Meissler

Resolution: needsinfofixed

Replying to Mariusz Felisiak:

Replying to Joe Meissler:

We can access schema_editor via with connection.schema_editor() as editor, similar to what is used in the migrate command. As for the atomic flag, BaseSchemaEditor has the atomic_migration attribute. You are correct that in the loader, the schema_editor is instantiated with migration.atomic. If, however, the editor itself always sets atomic = False, it doesn't matter that the migration is atomic.

So you have a 3rd party backend that ignores migration.atomic and always set SchemaEditor.atomic_migration = False and in the same time can roll back DDL (can_rollback_ddl=True), that's unusual. I'm not sure it's worth extra code 🤔.

Fair point. Perhaps the backend should implement its own DatabaseFeatures with can_rollback_ddl = False as well. I'll resolve as wontfix unless there's good reason not to.

comment:5 Changed 3 months ago by Mariusz Felisiak

Resolution: fixedwontfix

Thanks for the discussion 👍

Note: See TracTickets for help on using tickets.
Back to Top