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

operations on ManyToMany relations - do not use subquery when not necessary + avoid ER_CANT_UPDATE_USED_TABLE_IN_SF_OR_TRG error on mysql #2406

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

falkenhawk
Copy link
Contributor

@falkenhawk falkenhawk commented Apr 22, 2023

ported from ovos#11

1. performance optimization for operations on m2m relations

ManyToManyModifyMixin creates a subquery and moves all filters from the main query into it.

This is done so that we are able to innerJoin the join table to the query. Most SQL engines don't allow joins in updates or deletes. Join table is joined so that queries can reference the join table columns.

But, the subquery is not really needed in all types of operations

  • e.g. a query with just a findById(s) operation - usually coming from graph upsert
  • or when query builder is not operating on the join table but e.g. the target related table

2. Additionally, for mysql:

  • extract the subquery selecting related ids to separate query run before the main query
    to avoid ER_CANT_UPDATE_USED_TABLE_IN_SF_OR_TRG mysql error
    when executing a db trigger on a join table which updates related table

This workaround is only needed for MySQL.
It could possibly be applied to all DBMS, if proven necessary,
but others seem to handle such cases just fine.

https://stackoverflow.com/a/2314264/3729316

MySQL triggers can't manipulate the table they are assigned to. All other major DBMS support this feature so hopefully MySQL will add this support soon.
~ Cory House, 2010

@falkenhawk falkenhawk changed the title mysql: ManyToMany relations - do not use autogenerated subqueries where not necessary, avoid ER_CANT_UPDATE_USED_TABLE_IN_SF_OR_TRG error coming from mysql when triggers are attached to join table operations on ManyToMany relations - do not use subquery when not necessary + avoid ER_CANT_UPDATE_USED_TABLE_IN_SF_OR_TRG error on mysql Apr 22, 2023
1. performance optimization for operations on m2m relations
`ManyToManyModifyMixin` creates a subquery and moves all filters from the main query into it.
> This is done so that we are able to `innerJoin` the join table to the query. Most SQL engines don't allow joins in updates or deletes. Join table is joined so that queries can reference the join table columns.

But, the subquery is not really needed in all types of operations
- e.g. a query with just a findById(s) operation - usually coming from graph upsert
- or when query builder is not operating on the join table but e.g. the target related table

2. Additionally, for mysql:

- extract the subquery selecting related ids to separate query run before the main query
to avoid `ER_CANT_UPDATE_USED_TABLE_IN_SF_OR_TRG` mysql error
when executing a db trigger on a join table which updates related table

This workaround is only needed for MySQL.
It could possibly be applied to all DBMS, if proven necessary,
but others seem to handle such cases just fine.

https://stackoverflow.com/a/2314264/3729316
> MySQL triggers can't manipulate the table they are assigned to. All other major DBMS support this feature so hopefully MySQL will add this support soon.
> ~ Cory House, 2010
@lehni
Copy link
Collaborator

lehni commented Jun 29, 2023

This looks solid at first sight. I Do wonder though if this should be structured the same way as the library currently handles "anomalies" in Sqlite, using ManyToManySqliteModifyMixin and the sub-classes ManyToManyDeleteSqliteOperation, ManyToManyUnrelateSqliteOperation and ManyToManyUpdateSqliteOperation?

This would seem cleaner to me than starting to also branch inside ManyToManyModifyMixin.

Do you agree @falkenhawk, and would you be willing to work on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants