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

.transform() fails to drop column if table is part of a view #586

Open
simonw opened this issue Aug 18, 2023 · 5 comments
Open

.transform() fails to drop column if table is part of a view #586

simonw opened this issue Aug 18, 2023 · 5 comments
Labels
bug Something isn't working enhancement New feature or request

Comments

@simonw
Copy link
Owner

simonw commented Aug 18, 2023

I got this error trying to drop a column from a table that was part of a SQL view:

error in view plugins: no such table: main.pypi_releases

Upon further investigation I found that this pattern seemed to fix it:

def transform_the_table(conn):
    # Run this in a transaction:
    with conn:
        # We have to read all the views first, because we need to drop and recreate them
        db = sqlite_utils.Database(conn)
        views = {v.name: v.schema for v in db.views if table.lower() in v.schema.lower()}
        for view in views.keys():
            db[view].drop()
        db[table].transform(
            types=types,
            rename=rename,
            drop=drop,
            column_order=[p[0] for p in order_pairs],
        )
        # Now recreate the views
        for name, schema in views.items():
            db.create_view(name, schema)

So grab a copy of any view that might reference this table, start a transaction, drop those views, run the transform, recreate the views again.

I wonder if this should become an option in sqlite-utils? Maybe a recreate_views=True argument for table.tranform(...)? Should it be opt-in or opt-out?

Originally posted by @simonw in simonw/datasette-edit-schema#35 (comment)

@simonw simonw added bug Something isn't working enhancement New feature or request labels Aug 18, 2023
@simonw
Copy link
Owner Author

simonw commented Aug 18, 2023

More notes in here:

Not all Python/SQLite installations exhibit this problem by default!

It turns out this is controlled by the legacy_alter_table pragma: https://sqlite.org/pragma.html#pragma_legacy_alter_table

If that PRAGMA is turned on (default in newer SQLites) then alter table will error if you try to rename a table that is referenced in a view.

Here's a one-liner to test if it is on or not:

python -c 'import sqlite3; print(sqlite3.connect(":memory:").execute("PRAGMA legacy_alter_table").fetchall())'

@simonw
Copy link
Owner Author

simonw commented Aug 18, 2023

Options:

  • Provide a recreate_views: bool parameter to table.transform() controlling if views that might reference this table are stashed and dropped and recreated within a transaction as part of the operation. But should that be True or False by default?
  • Read that PRAGMA and automatically do that view workaround if it's turned on
  • Toggle that PRAGMA off for the duration of the .transform() operation and on again at the end. Does it only affect the current connection?
  • Try the transform() in a transaction, detect the "error in view", "no such table"error, if spotted then do the VIEW workaround and try again

I'm on the fence as to which of these I like the most. I'm tempted to go with the one which just drops VIEWS and recreates them all the time, because it feels simpler.

simonw added a commit to simonw/datasette-edit-schema that referenced this issue Aug 18, 2023
@simonw
Copy link
Owner Author

simonw commented Aug 18, 2023

I shipped the view recreating fix in datasette-edit-schema, so at least I can start exercising that fix and see if it has any weird issues.

@tobych
Copy link

tobych commented Apr 29, 2024

Just noting that the fix @simonw mentions is here in datasette-edit-schema.

I'm using views that reference tables that are changing, so I'm likely to want this fix.

@tobych
Copy link

tobych commented Apr 29, 2024

The documentation for the legacy_alter_table pragma states that the behavior it controls is per-connection, and not persistent.

Regardless, using the pragma seems a bad idea to me, because:

  • the feature it's controlling is a legacy behavior
  • the documentation says: "New applications should leave this flag turned off."

The recreate_views idea seems like it might be useful, but I reckon You Ain't Gonna Need It. If someone has a lot of views, and it takes ages to recreate them, perhaps. That seems highly unlikely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants