sqlalchemy-bot / test_alembic_1

0 stars 0 forks source link

Support for MySQL/MariaDB online migrations (support custom keyword arguments for operations) #423

Open sqlalchemy-bot opened 7 years ago

sqlalchemy-bot commented 7 years ago

Migrated issue, originally created by spanosgeorge ()

Hello, if I'm reading correctly the source code, right now alembic doesn't support running ALTER ONLINE TABLE DDL statements. Correct?

If indeed that's the case, are there plans to support it in the future?

Relevant documentation from MySQL: https://dev.mysql.com/doc/refman/5.6/en/alter-table-online-operations.html

sqlalchemy-bot commented 7 years ago

Michael Bayer (zzzeek) wrote:

Reading that doc, this is only for NDB cluster. Secondly, the documentation you link states pretty clearly that the ONLINE keyword is deprecated, so this is out of the gate a legacy option.

keywords like ONLINE can be added using recipes that intercept alembic.AlterTable to add that keyword, however I assume you want the ONLINE keyword conditionally, which would mean extending operations like op.alter_column() to do so. There seem to be a lack of hooks to easily intercept new arguments for a case like this without having to create an entirely new operation, which is also possible if you are truly in a bind for this (though you can just emit textual ALTER as well), so if anything I'd rather add extensibility for existing operations.

sqlalchemy-bot commented 7 years ago

Michael Bayer (zzzeek) wrote:

proposal is to add "contextual_ops" to all the ddl.base.AlterTable etc constructs, have dialect impls propagate these arguments from the operations to the DDL construct. All operations need to support **kw (many already do). Then you can write a @compiles(AlterTable, "mysql") recipe that looks inside of element.contextual_opts for mysql_online=True and this is easy.

a small part of the patch, would need to add **kw everywhere it needs to be as well as propagation in all of the impls:

#!diff

diff --git a/alembic/ddl/base.py b/alembic/ddl/base.py
index f4a525f..e74d116 100644
--- a/alembic/ddl/base.py
+++ b/alembic/ddl/base.py
@@ -24,15 +24,18 @@ class AlterTable(DDLElement):

     """

-    def __init__(self, table_name, schema=None):
+    def __init__(self, table_name, schema=None, contextual_opts=None):
         self.table_name = table_name
         self.schema = schema
+        self.contextual_opts = contextual_opts

 class RenameTable(AlterTable):

-    def __init__(self, old_table_name, new_table_name, schema=None):
-        super(RenameTable, self).__init__(old_table_name, schema=schema)
+    def __init__(self, old_table_name, new_table_name, schema=None,
+                 contextual_opts=None):
+        super(RenameTable, self).__init__(old_table_name, schema=schema,
+                                          contextual_opts=contextual_opts)
         self.new_table_name = new_table_name

@@ -41,8 +44,9 @@ class AlterColumn(AlterTable):
     def __init__(self, name, column_name, schema=None,
                  existing_type=None,
                  existing_nullable=None,
-                 existing_server_default=None):
-        super(AlterColumn, self).__init__(name, schema=schema)
+                 existing_server_default=None, contextual_opts=None):
+        super(AlterColumn, self).__init__(name, schema=schema,
+                                          contextual_opts=contextual_opts)
         self.column_name = column_name
         self.existing_type = sqltypes.to_instance(existing_type) \
             if existing_type is not None else None
diff --git a/alembic/ddl/mysql.py b/alembic/ddl/mysql.py
index 3af4fa6..538121e 100644
--- a/alembic/ddl/mysql.py
+++ b/alembic/ddl/mysql.py
@@ -45,7 +45,8 @@ class MySQLImpl(DefaultImpl):
                     default=server_default if server_default is not False
                     else existing_server_default,
                     autoincrement=autoincrement if autoincrement is not None
-                    else existing_autoincrement
+                    else existing_autoincrement,
+                    contextual_opts=kw
                 )
             )
         elif nullable is not None or \
@@ -64,7 +65,8 @@ class MySQLImpl(DefaultImpl):
                     default=server_default if server_default is not False
                     else existing_server_default,
                     autoincrement=autoincrement if autoincrement is not None
-                    else existing_autoincrement
+                    else existing_autoincrement,
+                    contextual_opts=kw
                 )
             )
         elif server_default is not False:
@@ -72,6 +74,7 @@ class MySQLImpl(DefaultImpl):
                 MySQLAlterDefault(
                     table_name, column_name, server_default,
                     schema=schema,
+                    contextual_opts=kw
                 )
             )

@@ -204,8 +207,8 @@ class MySQLImpl(DefaultImpl):

 class MySQLAlterDefault(AlterColumn):

-    def __init__(self, name, column_name, default, schema=None):
-        super(AlterColumn, self).__init__(name, schema=schema)
+    def __init__(self, name, column_name, default, schema=None, **kw):
+        super(AlterColumn, self).__init__(name, schema=schema, **kw)
         self.column_name = column_name
         self.default = default

@@ -217,8 +220,8 @@ class MySQLChangeColumn(AlterColumn):
                  type_=None,
                  nullable=None,
                  default=False,
-                 autoincrement=None):
-        super(AlterColumn, self).__init__(name, schema=schema)
+                 autoincrement=None, **kw):
+        super(AlterColumn, self).__init__(name, schema=schema, **kw)
         self.column_name = column_name
         self.nullable = nullable
         self.newname = newname
sqlalchemy-bot commented 7 years ago

spanosgeorge () wrote:

Hey, thanks for the prompt response!

Indeed, I should have linked probably to MariaDB's doc: https://mariadb.com/kb/en/mariadb/alter-table/.

For MariaDB ALTER ONLINE TABLE is just a synonym for ALTER TABLE ... LOCK=NONE. I will try to see if I can get away with any of the temporary solutions you suggested in your first response, until your patch lands to a proper alembic release.

Thanks again.

sqlalchemy-bot commented 7 years ago

Changes by Michael Bayer (zzzeek):

sqlalchemy-bot commented 6 years ago

Changes by Michael Bayer (zzzeek):