Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
sqlalchemy-bot opened this issue Mar 23, 2017 · 5 comments

Comments

@sqlalchemy-bot
Copy link
Owner

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
Copy link
Owner Author

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
Copy link
Owner Author

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
Copy link
Owner Author

Changes by Michael Bayer (zzzeek):

  • added labels: op directives
  • set milestone to "fasttrack"
  • changed title from "Support for MySQL/MariaDB online migrations" to "Support for MySQL/MariaDB online migrations (suppo"

@sqlalchemy-bot
Copy link
Owner Author

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
Copy link
Owner Author

Changes by Michael Bayer (zzzeek):

  • changed milestone from "fasttrack" to "tier 1"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant