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

Invalid SQL is generated when attempting to change server default to CURRENT_TIMESTAMP #564

Closed
GoldstHa opened this issue May 18, 2019 · 9 comments

Comments

Projects
None yet
3 participants
@GoldstHa
Copy link

commented May 18, 2019

op.alter_column('micros_menu_items', 'warehouse_last_update',
            existing_type=mysql.TIMESTAMP(),
            server_default=sa.text('CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP'),
            existing_nullable=True)

emits this:

ALTER TABLE otg.micros_menu_items ALTER COLUMN warehouse_last_update SET DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP;

...which is invalid.

This would be syntactically valid, but still fail:

ALTER TABLE otg.micros_menu_items ALTER COLUMN warehouse_last_update SET DEFAULT 'CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP';

This seems to be the only valid way:

ALTER TABLE otg.micros_menu_items
CHANGE COLUMN warehouse_last_update warehouse_last_update TIMESTAMP NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP;

I understand that in older versions of MySQL simply making a TIMESTAMP column NOT NULL would have the same effect, but newer versions MySQL need that default clause in order to have an auto-initializing/updating timestamp column.

@zzzeek

This comment has been minimized.

Copy link
Member

commented May 18, 2019

we don't have explicit support for MySQL's "ON UPDATE" clause right now, this is in SQLAlchemy as sqlalchemy/sqlalchemy#4652 where we will add a mysql_on_update and we also welcome pull requests in this regard, for the Alembic side there would also be a mysql_on_update parameter consumed by the **kw of AlterColumnOp. Any solution that's in explicit support of ON UPDATE needs the upstream sqlalchemy issue implemented first.

The "CHANGE" syntax is part of the MySQL Alembic dialect right now however it only takes effect when the column's name is changing. If you add "name='warehouse_last_update'" to your operation it might emit what you want here, please try that.

additionally, need to know the scope of the syntax here, can you please share exact MySQL / MariaDB version in use, additionally can you confirm that without the "ON UPDATE" part, the existing ALTER TABLE syntax for change of server defaut still works ? I'm sort of guessing this is MySQL 8 where they have changed all kinds of syntaxes.

@zzzeek zzzeek added the mysql label May 18, 2019

@zzzeek zzzeek added this to the fasttrack milestone May 18, 2019

@GoldstHa

This comment has been minimized.

Copy link
Author

commented May 19, 2019

"name='warehouse_last_update'" worked. Thank you!

I'm actually working with both MySQL 5.5.59 and 8.0.16. The results are the same on both. The ALTER TABLE ... ALTER COLUMN ... SET DEFAULT ...; syntax is correct for other datatypes. ALTER TABLE otg.micros_menu_items ALTER COLUMN warehouse_last_update SET DEFAULT CURRENT_TIMESTAMP; doesn't work and neither does ALTER TABLE otg.micros_menu_items ALTER COLUMN warehouse_last_update SET DEFAULT 'CURRENT_TIMESTAMP';

sqlalchemy-bot pushed a commit that referenced this issue May 20, 2019

Create alter column backend test fixture
For more expedient confirmation of op functionality,
start building generalized backend fixtures for ops.

Add basic round trip tests for alter column, obviously
disabled on SQLite.   Ensure this passes for the full
span of MySQL, Oracle, SQL Server on CI (PG is fine).
Next we can begin adding tests for all the MySQL issues
that are coming up such as #564, where regular ALTER COLUMN
is not consistently implemented based on datatypes etc.

Change-Id: I00ef174aa791ce7baeb764c5915591cd095deae0
@zzzeek

This comment has been minimized.

Copy link
Member

commented May 20, 2019

mariadb supports:

ALTER TABLE x ALTER COLUMN colname SET DEFAULT CURRENT_TIMESTAMP

MySQL does not. will now try the CHANGE syntax, and see if MariaDB supports that, I bet it doesn't and we have to switch based on mysql / mariadb.

@zzzeek

This comment has been minimized.

Copy link
Member

commented May 20, 2019

so you can use CHANGE for all server default changes on both DBs. But I'm not able to find any reasoning for what is different and I don't think I'm going to do it that way, because I bet it does something very different internally like lock the table more aggressively, or something like that. but nobody knows. so at the moment leaning towards regexp for CURRENT_TIMESTAMP / NOW w datetime column only.

@zzzeek

This comment has been minimized.

Copy link
Member

commented May 20, 2019

OK "ON UPDATE" is even bundled into how we reflect the default on MySQL in SQLAlchemy right now, that makes sqlalchemy/sqlalchemy#4652 a lot more weird

@sqla-tester

This comment has been minimized.

Copy link
Collaborator

commented May 20, 2019

Mike Bayer has proposed a fix for this issue in the master branch:

Use CHANGE COLUMN for MySQL / MariaDB DateTime server default change https://gerrit.sqlalchemy.org/1277

@GoldstHa

This comment has been minimized.

Copy link
Author

commented May 20, 2019

Both ALTER TABLE ... CHANGE COLUMN ... ; and ALTER TABLE ... MODIFY COLUMN ...; are valid for setting the default value of a TIMESTAMP column to CURRENT_TIMESTAMP. When I run them and compare status variables, Innodb_rows_inserted goes up for both, but not by the amount I'd expect if it were doing a full table rebuild... so I'm not sure I'm on the right track with that. Duration seems to be roughly the same though. From what I've read, MODIFY COLUMN does do a full table rebuild.

@GoldstHa

This comment has been minimized.

Copy link
Author

commented May 20, 2019

Beginning with 5.6, there's support for online DDL and beginning with 8.0 there's support for atomic DLL, though at the moment I don't have much experience with either. When I do, I may make some suggestions for making safer, non-blocking DDL statements.

I've also been working on a heuristic for arranging operations when using --autogenerate in a way that is guaranteed to not fail. With transactional DDL, I assume it's no big deal. But with non-transactional DDL it's a real pain to 'rollback' only those changes that went through.

@zzzeek

This comment has been minimized.

Copy link
Member

commented May 20, 2019

well for this issue it's between ALTER COLUMN and CHANGE COLUMN so I have it doing the CHANGE syntax based on a conservative check only.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.