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

Entity with multiple @ManyToOne's with primary:true causes extra indexes to be generated #760

Closed
msheakoski opened this issue Aug 19, 2020 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@msheakoski
Copy link

Describe the bug
When generating a migration for an entity with multiple @ManyToOne properties that are also primary keys, an extra index for each @ManyToOne column is generated. Using the OrderItem entity from the example on https://mikro-orm.io/docs/composite-keys/#use-case-3-join-table-with-metadata the following is generated:

this.addSql('create index "order_item_order_id_index" on "order_item" ("order_id");');
this.addSql('create index "order_item_product_id_index" on "order_item" ("product_id");');
this.addSql('alter table "order_item" add constraint "order_item_pkey" primary key ("order_id", "product_id");');

The last line which adds the primary key is enough to create a unique constraint + index. The indexes on individual columns are not needed. I tried adding { index: false } to the @ManyToOne options but it still resulted in the individual indexes being generated.

Expected behavior
Only the "add constraint" SQL for composite primary key should be present, with no indexes on order_id and product_id.

Versions

Dependency Version
node 14.8.0
typescript 3.9.7
mikro-orm 4.0.0-rc.2
your-driver postgresql
@msheakoski msheakoski added the bug Something isn't working label Aug 19, 2020
@B4nan
Copy link
Member

B4nan commented Aug 19, 2020

Will need to verify, but i believe the indexes were needed. If not needed, then definitely something you should want anyway.

Indexing Foreign Keys

The first feature is auto indexing of foreign keys. Most people incorrectly assume that databases index foreign keys by default. Well, they don't. Primary keys are auto indexed, but foreign keys are not. This means any query based on the foreign key will be doing full table scans. This is any OneToMany, ManyToMany or ElementCollection relationship, as well as many OneToOne relationships, and most queries on any relationship involving joins or object comparisons. This can be a major perform issue, and you should always index your foreign keys fields.

https://stackoverflow.com/questions/970562/postgres-and-indexes-on-foreign-keys-and-primary-keys

@msheakoski
Copy link
Author

msheakoski commented Aug 19, 2020

In the top-voted reply, https://stackoverflow.com/a/970605/2612450, it says:

Note that if you use primary-foreign-keys, like 2 FK's as a PK in a M-to-N table, you will have an index on the PK and probably don't need to create any extra indexes.

which supports what I was trying to explain in the issue description. The reply that you quoted is correct. Foreign keys are not automatically indexed by default. Any column(s) marked as a primary key, even if they are foreign, are automatically indexed and given a unique constraint.

@msheakoski
Copy link
Author

msheakoski commented Aug 19, 2020

Another good point in the reply that you linked to is that creating an index on a foreign key should not be forced. Whether you decide for the default MikroORM behavior to be opt-in or opt-out is up to you, but the end user should be able to disable indexes with { index: false } if they are created by default.

Since adding index incurs performance penalty on all inserts, updates and deletes, and lots of foreign keys could really add up in this case. That's why this behavior is opt-in I guess - developer should make conscious choice in this matter. There could also be cases when foreign key is used to enforce data integrity, but is not queried often or queried at all - in this case performance penalty of index would be for nothing

@B4nan
Copy link
Member

B4nan commented Aug 19, 2020

Makes sense. This originated probably with MySQL implementation where indexes are required for FKs (at least afaik).

@B4nan
Copy link
Member

B4nan commented Aug 20, 2020

Ok I can confirm this is not needed for composite keys, not even for MySQL or SQLite.

@B4nan B4nan closed this as completed in 91b38cb Aug 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants