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

Fix all usecases where deadlocks can happen (for MySQL versions above 5 and MariaDB) #15143

Merged
merged 1 commit into from Dec 14, 2022

Conversation

petersg83
Copy link
Contributor

What does it do?

The same query than the one used for MySQL 5 (using a temporary table) is now used also for MySQL 8 and MariaDB

Why is it needed?

Deadlock issues found with the custom code for MySQL 5 were also found on the code used for MySQL 8 and MariaDB.
The custom code for MySQL 5 was recently fixed in #15034.
Since the custom code for MySQL 5 also works for MySQL 8 and MariaDB, it becomes now the default code for MySQL 5, MySQL 8 and MariaDB

How to test it?

Follow the steps described in #14937 by using MySQL 8 or MariaDB

Related issue(s)/PR(s)

fix #14937

@petersg83 petersg83 added source: core:database Source is core/database package pr: fix This PR is fixing a bug labels Dec 9, 2022
@petersg83 petersg83 added this to the 4.5.4 milestone Dec 9, 2022
@codecov
Copy link

codecov bot commented Dec 9, 2022

Codecov Report

Base: 59.88% // Head: 59.88% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (aef49c5) compared to base (2cdd96f).
Patch coverage: 4.16% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15143   +/-   ##
=======================================
  Coverage   59.88%   59.88%           
=======================================
  Files        1348     1348           
  Lines       32800    32799    -1     
  Branches     6255     6255           
=======================================
  Hits        19642    19642           
+ Misses      11303    11302    -1     
  Partials     1855     1855           
Flag Coverage Δ
back 50.09% <4.16%> (+<0.01%) ⬆️
front 64.33% <ø> (ø)
unit_back 50.09% <4.16%> (+<0.01%) ⬆️
unit_front 64.33% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...e/database/lib/entity-manager/regular-relations.js 10.09% <4.16%> (+0.27%) ⬆️
...er/services/schema-builder/content-type-builder.js 8.52% <0.00%> (-0.14%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@nathan-pichon nathan-pichon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added some comments, but I have not yet been able to test the branch.

Comment on lines +206 to +231
const { joinTable } = attribute;
const { joinColumn, inverseJoinColumn, orderColumnName, inverseOrderColumnName } = joinTable;
const update = [];
const updateBinding = [];
const select = ['??'];
const selectBinding = ['id'];
const where = [];
const whereBinding = [];

if (hasOrderColumn(attribute) && id) {
update.push('?? = b.src_order');
updateBinding.push(orderColumnName);
select.push('ROW_NUMBER() OVER (PARTITION BY ?? ORDER BY ??) AS src_order');
selectBinding.push(joinColumn.name, orderColumnName);
where.push('?? = ?');
whereBinding.push(joinColumn.name, id);
}

if (hasInverseOrderColumn(attribute) && !isEmpty(inverseRelIds)) {
update.push('?? = b.inv_order');
updateBinding.push(inverseOrderColumnName);
select.push('ROW_NUMBER() OVER (PARTITION BY ?? ORDER BY ??) AS inv_order');
selectBinding.push(inverseJoinColumn.name, inverseOrderColumnName);
where.push(`?? IN (${inverseRelIds.map(() => '?').join(', ')})`);
whereBinding.push(inverseJoinColumn.name, ...inverseRelIds);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we can split this in multiple functions to improve the readability, but looks good enough to be merged.

Copy link
Contributor

@Marc-Roig Marc-Roig Dec 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, and I find it a little bit difficult to follow when having multiple variables to keep track.

Maybe having a more abstract method such as the following could help:

if (hasInverseOrderColumn(attribute) && !isEmpty(inverseRelIds)) {
  update.push("?? = b.inv_order", inverseOrderColumnName);
  select.push(
    "ROW_NUMBER() OVER (PARTITION BY ?? ORDER BY ??) AS inv_order",
    inverseJoinColumn.name,
    inverseOrderColumnName
  );
  where.push(
    `?? IN (${inverseRelIds.map(() => "?").join(", ")})`,
    inverseJoinColumn.name,
    ...inverseRelIds
  );
}

So then you would do:

      await db.connection
        .raw(
          `UPDATE ?? as a
              SET ${update.getQuery().join(', ')}
              FROM (
                SELECT ${select.getQuery()join(', ')}
                FROM ??
                WHERE ${where.getQuery()join(' OR ')}
              ) AS b
              WHERE b.id = a.id`,
          [joinTableName, ...update.getBindings(), ...select.getBindings(), joinTableName, ...where.getBindings()]
        )
        .transacting(trx);

@Marc-Roig
Copy link
Contributor

I have not managed to replicate the issue using MySQL 8 nor MariaDB . I followed the steps described in #14937 . Could we look at this together some point?

It also interest me to see if the transactions PR #14389 introduces deadlocks too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:database Source is core/database package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ER_LOCK_DEADLOCK error when trying to save an entity with multiple nested components
3 participants