Alembic's --autogenerate tries to remove primary key

[tailhook]

I.e. I see the following in the log:

INFO  [alembic.autogenerate.compare] Detected removed index 'primary' on 'users'

And the following operations in generated upgrade script:

    op.drop_index('primary', table_name='users')

I think the list of indexes that cockroachdb (the server or python bindings) generates is somehow different from the one of postgres, but I haven't compared them directly.

[lgo]

Hey @tailhook. Your right that cockroachdb does some things differently. In particular, cockroachdb creates and lists the primary index whereas postgres does not expose the primary index when drivers ask for all indexes.

If you're using SQLAlchemy, you should be able to add the index into the model to prevent alembic from dropping it. (The name will be primary, it will be unique, and will consist of any columns you specified when the table was created).

Something to watch out for is if you use SQLAlchemy to create the table in the database, e.g. mytable.create(engine). It will try to create the primary key using PRIMARY KEY (col1, col2, ...) and also UNIQUE INDEX 'primary' (col1, col2, ...) which will fail.

[tailhook]

If you're using SQLAlchemy, you should be able to add the index into the model to prevent alembic from dropping it

Sounds weird. Shouldn't we filter primary index in metadata, or fix comparison function somehow? How non-postgresql bindings work in this respect?

[tailhook]

I mean there are better hooks in alembic to filter out primary keys, except creating them as a separate indexes (I have primary_key=True on a column). But I think it should work out of the box.

[lgo]

Yea, that was a suggestion for a possible workaround for now. I had completely forgot about some of the configuration bits alembic has, so you are right that filtering metadata would be better! :)

I'm not quite sure if the environment config include_object filters both data from models and metadata pulled from the data, so it might be worth a try to add this to env.py:

def include_object(object, name, type_, reflected, compare_to):
    if (type_ in ["index", "unique_constraint"] and
        not reflected and
        name == "primary"):
        return False
    else:
        return True

context.configure(
    # ...
    include_object = include_object
)

I totally agree with you that it should work out of the box but unfortunately, we haven't gotten a great coverage of all the drivers out there. I'll try to get to this when I get the chance to :).

(on our end, it should just be a matter of filtering out the indexes in https://github.com/zzzeek/alembic/blob/master/alembic/ddl/postgresql.py#L148-L188)

[bdarnell]

@LEGO shouldn't we be filtering these out before they get to alembic? (probably in CockroachDBDialect.get_indexes, or wherever it's getting its index information from) I'm surprised that sqlalchemy's test suite doesn't catch this since it does so much reflection.

[lgo]

Ah, yea that is the more appropriate change. Alembic does get the info from get_indexes when it compares.

The fact that the test suite doesn't catch this is alarming since there are definitely tests that will fail with cockroach (e.g. asserting index counts). I'm guessing we either aren't running those specific postgres dialect tests, or the reflection test isn't running because we aren't postgres?

I'd be happy to fix this and also look into getting the tests working. (It looks like they were disabled? was that just because of circleci problems?)

[bdarnell]

That test appears to do a string match on the dialect name to only run on postgresql. I'm not sure if there's a clean way for us to run those tests on cockroach.

The tests were disabled in the past, but as far as I know they're back now.

[sibblegp]

Autogenerate also does a few other things:

• Recreate Foreign Keys
• Drop indexes automatically created for foreign keys
• Does not remove the foreign key constraint when deleting a foreign key (causing an error on migration)

It's very frustrating to have to go through and manually clean out every autogenerate considering how well it works on postgres. I suppose I could add the indexes to the model but that seems a bit challenging and time consuming, especially considering how large of a model I use.

Here is an example of the code generated:

op.drop_index('carrier_monthly_reports_auto_index_fk_carrier_id_ref_carriers', table_name='carrier_monthly_reports') op.drop_index('carrier_monthly_reports_auto_index_fk_mining_pool_id_ref_mining_pools', table_name='carrier_monthly_reports') op.drop_index('primary', table_name='carrier_monthly_reports') op.create_foreign_key(None, 'carrier_monthly_reports', 'carriers', ['carrier_id'], ['id']) op.create_foreign_key(None, 'carrier_monthly_reports', 'mining_pools', ['mining_pool_id'], ['id']) op.drop_index('carrier_payments_auto_index_fk_carrier_id_ref_carriers', table_name='carrier_payments') op.drop_index('primary', table_name='carrier_payments') op.create_foreign_key(None, 'carrier_payments', 'carriers', ['carrier_id'], ['id'])

[MSeal]

Is there any update on this issue? This is making automatic migration generation impossible to use as our model count grows.

[rafiss]

@MSeal Thanks for resurfacing this. I created PR #124 which should address this

[MSeal]

Awesome, thanks for digging into the issue!