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

accommodate self-referential FKs, document best practice for PRAGMA FOREIGN KEYS in batch #345

Closed
sqlalchemy-bot opened this issue Dec 11, 2015 · 14 comments
Milestone

Comments

@sqlalchemy-bot
Copy link

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Migrated issue, originally created by Christian Benke (@peletiah)

On a sqlite-DB I'm trying to alter a table with

with op.batch_alter_table('resources', schema=None) as batch_op:
        batch_op.add_column(sa.Column('parent_id', sa.BigInteger(),
                    sa.ForeignKey('resources.resource_id',
                      onupdate='CASCADE', ondelete='SET NULL')))

but the foreign key I end up with points to "_alembic_patch_temp" instead of "resources":

CONSTRAINT fk_resources_parent_id_resources FOREIGN KEY(parent_id) REFERENCES _alembic_batch_temp (resource_id) ON DELETE SET NULL ON UPDATE CASCADE,

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Michael Bayer (@zzzeek) wrote:

the table "_alembic_batch_temp" gets renamed to "resources" as the final step. Does that part fail ? or is sqlite so lame that it leaves the FK pointing to nothing?

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Christian Benke (@peletiah) wrote:

Unfortunately that step seems to fail, the reference to "_alembic_batch_temp" is the final result. Here's the full patch I apply (Adapted from ziggurat_foundations):

"""add id/parent id to resource structure

Revision ID: 5c84d7260c5
Revises: 24ab8d11f014
Create Date: 2011-11-11 00:09:09.624704

"""

# downgrade revision identifier, used by Alembic.
revision = '5c84d7260c5'
down_revision = '24ab8d11f014'

from alembic.op import *
import sqlalchemy as sa

def upgrade():
    add_column('resources', sa.Column('parent_id', sa.BigInteger(),
        sa.ForeignKey('resources.resource_id',
                      onupdate='CASCADE', ondelete='SET NULL')))

def downgrade():
    pass

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Michael Bayer (@zzzeek) wrote:

ok in your env.py, what happens if you emit "PRAGMA foreign_keys=ON" on the connection? Basically if this proceeds without any errors, then what we might do is make self-ref FKs just refer to the as-yet-nonexistent table. but this is weird and might even be considered a SQLite bug (well of course it is, but a bug that sqlite would actually care about...which is less likely).

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Michael Bayer (@zzzeek) wrote:

ah - yes this is the solution and I'd say we just document it:

https://www.sqlite.org/lang_altertable.html

If foreign key constraints are enabled when a table is renamed, then any REFERENCES clauses in any table (either the table being renamed or some other table) that refer to the table being renamed are modified to refer to the renamed table by its new name.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Christian Benke (@peletiah) wrote:

Yes, that did the trick, thanks a lot! For reference how to add "PRAGMA foreign_keys=ON to env.py":

import sqlite3

from sqlalchemy.engine import Engine
from sqlalchemy import event

@event.listens_for(Engine, "connect")
def set_sqlite_pragma(dbapi_connection, connection_record):
    if type(dbapi_connection) is sqlite3.Connection:  # play well with other DB backends
       cursor = dbapi_connection.cursor()
       cursor.execute("PRAGMA foreign_keys=ON")
       cursor.close()


@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Michael Bayer (@zzzeek) wrote:

right but funny thing, with FKs on, now batch will fail for a table that has other tables pointing to it. and that is much harder to solve.

so here i think there's the need to:

  1. document what PRAGMA FOREIGN KEYS should be set to, and how, and it might need to be off in the general case

  2. add some behavior / flag that accommodates at least when PRAGMA FOREIGN KEYS is off and sets the FK to the original table name for self-referential

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Changes by Michael Bayer (@zzzeek):

  • changed title from "batch_op.add_column creates reference to "_alembic" to "accommodate self-referential FKs document best pra"
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Changes by Michael Bayer (@zzzeek):

  • added labels: batch migrations
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Changes by Michael Bayer (@zzzeek):

  • set milestone to "tier 1"
@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 11, 2015

Michael Bayer (@zzzeek) wrote:

as a workaround for now, you can temporarily set PRAGMA FOREIGN KEYS in your migration script using op.execute(), then turn it off. because if you leave it on , subsequent batch migrations will likely break for tables that refer to a batch-migrated table, since we're dropping a table that they refer to.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 15, 2015

Michael Bayer (@zzzeek) wrote:

so yeah the right way to do this is going to break it for you, it's that we generate the FK constraint pointing to the ultimate table name, which will break if you have PRAGMA FOREIGN KEYS turned on. This makes it consistent with non-self-referential FKs which will never work w PRAGMA FKS on. So that'll be 0.8.4.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 15, 2015

Christian Benke (@peletiah) wrote:

Ok, thanks! I'll upgrade then. For now I've switched to postgres, which is not such a pita.

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 15, 2015

Michael Bayer (@zzzeek) wrote:

  • Batch mode generates a FOREIGN KEY constraint that is self-referential
    using the ultimate table name, rather than _alembic_batch_temp.
    When the table is renamed from _alembic_batch_temp back to the
    original name, the FK now points to the right name. This
    will not work if referential integrity is being enforced (eg. SQLite
    "PRAGMA FOREIGN_KEYS=ON") since the original table is dropped and
    the new table then renamed to that name, however this is now consistent
    with how foreign key constraints on other tables already operate
    with batch mode; these don't support batch mode if referential integrity
    is enabled in any case.
    fixes #345

da7d599

@sqlalchemy-bot
Copy link
Author

@sqlalchemy-bot sqlalchemy-bot commented Dec 15, 2015

Changes by Michael Bayer (@zzzeek):

  • changed status to closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants
You can’t perform that action at this time.