From d2218daf16e25f87ff220e0063e19730b7bf63b1 Mon Sep 17 00:00:00 2001 From: Marcin L <62071967+marcinlee@users.noreply.github.com> Date: Thu, 21 Jul 2022 13:50:33 +0200 Subject: [PATCH] m2m relations - do not use subquery when not necessary 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 --- .../manyToMany/ManyToManyModifyMixin.js | 76 +++++++++++++++++-- 1 file changed, 69 insertions(+), 7 deletions(-) diff --git a/lib/relations/manyToMany/ManyToManyModifyMixin.js b/lib/relations/manyToMany/ManyToManyModifyMixin.js index 93f741d93..2526d1571 100644 --- a/lib/relations/manyToMany/ManyToManyModifyMixin.js +++ b/lib/relations/manyToMany/ManyToManyModifyMixin.js @@ -1,6 +1,10 @@ 'use strict'; const { ManyToManyFindOperation } = require('./find/ManyToManyFindOperation'); +const { isMySql } = require('../../utils/knexUtils'); + +const FindByIdSelector = /^findByIds?$/; +const RelateUnrelateSelector = /relate$/; // This mixin contains the shared code for all modify operations (update, delete, relate, unrelate) // for ManyToManyRelation operations. @@ -10,6 +14,8 @@ const { ManyToManyFindOperation } = require('./find/ManyToManyFindOperation'); // 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. +// +// If the subquery is not needed at all (e.g. the query has only a findById(s) operation - usually coming from graph upsert) - skip it const ManyToManyModifyMixin = (Operation) => { return class extends Operation { constructor(...args) { @@ -26,7 +32,7 @@ const ManyToManyModifyMixin = (Operation) => { onBuild(builder) { this.modifyFilterSubquery = this.createModifyFilterSubquery(builder); - if (this.modifyMainQuery) { + if (this.modifyMainQuery && this.modifyFilterSubquery) { // We can now remove the where and join statements from the main query. this.removeFiltersFromMainQuery(builder); @@ -38,6 +44,22 @@ const ManyToManyModifyMixin = (Operation) => { } createModifyFilterSubquery(builder) { + // Check if the subquery is needed + // - it may not be, if there are no operations other than findById(s) on the main query + // and proceed only if passed builder operates on the joinTable + if (builder.modelClass() === this.relation.joinTableModelClass) { + const checkQuery = builder + .clone() + .toFindQuery() + .modify(this.relation.modify) + .clear(RelateUnrelateSelector) + .clear(FindByIdSelector) + .clearOrder(); + if (checkQuery.isSelectAll()) { + return null; + } + } + const relatedModelClass = this.relation.relatedModelClass; const builderClass = builder.constructor; @@ -73,15 +95,55 @@ const ManyToManyModifyMixin = (Operation) => { applyModifyFilterForJoinTable(builder) { const joinTableOwnerRefs = this.relation.joinTableOwnerProp.refs(builder); const joinTableRelatedRefs = this.relation.joinTableRelatedProp.refs(builder); - - const relatedRefs = this.relation.relatedProp.refs(builder); const ownerValues = this.owner.getProps(this.relation); - const subquery = this.modifyFilterSubquery.clone().select(relatedRefs); + if (this.modifyFilterSubquery) { + // if subquery is used (in a non-find query) + const relatedRefs = this.relation.relatedProp.refs(builder); + const subquery = this.modifyFilterSubquery.clone().select(relatedRefs); + + if (isMySql(builder.knex())) { + // and only 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 + builder + .runBefore(() => subquery.execute()) + .runBefore((related, builder) => { + if (!related.length) { + builder.resolve([]); + return; + } + builder.whereInComposite( + joinTableRelatedRefs, + related.map((m) => m.$values(this.relation.relatedProp.props)) + ); + }); + } else { + builder.whereInComposite(joinTableRelatedRefs, subquery); + } + } else if (builder.parentQuery()) { + // if subquery is not used: + // rewrite findById(s) from related table to join table + builder.parentQuery().forEachOperation(FindByIdSelector, (op) => { + if (op.name === 'findByIds') { + builder.whereInComposite(joinTableRelatedRefs, op.ids); + } else { + builder.whereComposite(joinTableRelatedRefs, op.id); + } + }); + } - return builder - .whereInComposite(joinTableRelatedRefs, subquery) - .whereInComposite(joinTableOwnerRefs, ownerValues); + return builder.whereInComposite(joinTableOwnerRefs, ownerValues); } toFindOperation() {