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
bulk insert w/ SQL Server and no-pk table in offline mode should not render SET IDENTITY INSERT #495
Comments
Changes by Wil Tan:
|
Michael Bayer (@zzzeek) wrote: Not able to reproduce the second case: migration:
output running in online mode:
in online mode, the SQLAlchemy dialect is doing the work here and it knows to turn on IDENTITY INSERT if the table has an autoincrement column and the given values contain that column. OTOH, if you are doing it like the documentation implies:
unfortunately that can't work for SQL Server because the lower-case "column()" / "table()" system has no primary key concept (or autoincrement column), thus the dialect doesn't know about a primary key and correctly doesn't turn on IDENTITY INSERT (the way you want the first case to work). |
Wil Tan wrote: Thanks for the tip! I was using lower case |
Michael Bayer (@zzzeek) wrote: sure, the first one sounds like it should be fixed as I noticed this is just a hardcoded rule in the Alembic impl in any case. |
Michael Bayer (@zzzeek) wrote: I'm also not totally happy that the lowercase
e.g. we add the ability to send execution options to bulk_insert and we get the SQL Server dialect to allow for this hint to be explicit. |
Wil Tan wrote: If this is the only use case for scrapping / changing the lowercase One way that could be useful is to warn user about the mixed use of uppercase / lowercase
|
Michael Bayer (@zzzeek) wrote: well we can always fix one thing at a time, I'm waiting on you for a PR right? there is absolutely no hurry. |
Wil Tan wrote: Sounds good, I'll submit a PR for the offline case. Do you prefer a PR in Github rather than Bitbucket? |
Michael Bayer (@zzzeek) wrote: github is better. I'd ike to get off bitbucket completely but I'm waiting on github to assist in that. |
Michael Bayer (@zzzeek) wrote: updated the title to the actual thing we want to fix here. |
Changes by Michael Bayer (@zzzeek):
|
I'm not great at writing unit tests right now in python, so I haven't created a merge request, but I was able to manually test the following code replacement with and without a database connection and it properly identifies when the column has an identity column and set the from alembic import op
import sqlalchemy as sa
from sqlalchemy.dialects import mssql
from alembic.ddl.mssql import MSSQLImpl
from sqlalchemy.schema import Sequence
def bulk_insert(self, table, rows, **kw):
id_column = table._autoincrement_column
insert_has_identity = (id_column is not None) and (
not isinstance(id_column.default, Sequence)
)
if self.as_sql and insert_has_identity:
self._exec(
"SET IDENTITY_INSERT %s ON"
% self.dialect.identifier_preparer.format_table(table)
)
super(MSSQLImpl, self).bulk_insert(table, rows, **kw)
self._exec(
"SET IDENTITY_INSERT %s OFF"
% self.dialect.identifier_preparer.format_table(table)
)
else:
super(MSSQLImpl, self).bulk_insert(table, rows, **kw)
MSSQLImpl.bulk_insert = bulk_insert I tested it with the following: table_name = 'MY_EXAMPLE'
my_example_table = op.create_table(
table_name,
sa.Column('my_id', sa.INTEGER, nullable=False),
sa.Column('my_value', sa.VARCHAR(length=4), nullable=False),
sa.PrimaryKeyConstraint('my_id')
)
op.bulk_insert(
my_example_table,
[
{'my_value': '800A'},
]
) b) an integer that is not an identity table_name = 'MY_EXAMPLE'
my_example_table = op.create_table(
table_name,
sa.Column('my_id', sa.INTEGER, nullable=False, autoincrement=False),
sa.Column('my_value', sa.VARCHAR(length=4), nullable=False),
sa.PrimaryKeyConstraint('my_id')
)
op.bulk_insert(
my_example_table,
[
{'my_id': 2, 'my_value': '800A'},
]
) c) something that is not an integer (and thus cannot be an identity) table_name = 'MY_EXAMPLE'
my_example_table = op.create_table(
table_name,
sa.Column('my_id', mssql.UNIQUEIDENTIFIER, server_default=sa.func.newid(), nullable=False),
sa.Column('my_value', sa.VARCHAR(length=4), nullable=False),
sa.PrimaryKeyConstraint('my_id')
)
op.bulk_insert(
my_example_table,
[
{'my_value': '800A'},
]
) |
Migrated issue, originally created by Wil Tan
In offline mode, Alembic emits IDENTITY INSERT mode control statements. However, in online mode, it does not do so.
There are a few shortcomings that trips up my use cases:
in offline mode, sometimes I
bulk_insert
into a table with no auto-increment key. TheSET IDENTITY INSERT <table> ON
statement will result in an error in MSSQL.in online mode, sometimes I do want to insert fixed primary key values for auto-increment fields, but because Alembic does not turn on IDENTITY INSERT mode, MSSQL throws an error.
Would it make sense to change the current behaviour in alembic/ddl/mssql.py such that it emits
SET IDENTITY INSERT
statements if the rows data contains thetable._autoincrement_column
, regardless of offline/online mode?I'm happy to work on a pull request if you agree with the approach.
The text was updated successfully, but these errors were encountered: