mysql 5.7 でdeadlockが発生

[nobuhiko]

ec-cube2/data/class/helper/SC_Helper_Purchase.php

Line 682 in 6d283fe

$objQuery->delete($table, $where, array($order_id));

mysql 5.7 でdeadlockがまれに発生する可能性がある、らしい
https://www.slideshare.net/yuyamada777/delete-77702365

[nanasess]

行の存在を確認してから DELETE するのが良い感じですかね

[nobuhiko]

そうみたいです

[seasoftjapan]

デッドロックを避け DELETE が走らなかったところで、片方のトランザクションの INSERT は通常キー重複で落ちる気がします。EC-CUBE の動作上はシステムエラーなので少なくともフロントでは差が無い気がします。むしろ落ちずに通ってしまう事があったら、その方が大問題なので、その観点では MySQL は PostgreSQL より安全方向に振った標準設定であり、#1097 はそれを殺す変更になる気がします。

安全方向に振る価値が無く、デッドロックを避けるのが最優先であれば、PostgreSQL 同様に READ COMMITTED にする方がスマートだと思います。

SET SESSION TRANSACTION ISOLATION LEVEL READ COMMITTED;

#1097 では対応できない DELETE 以外の行ロックとの組み合わせにも対応します。

[seasoftjapan]

上とは別の観点で、#1097 の手法で行くにしても、PostgreSQL を巻き込まない配慮は可能そうに思います。

[nanasess]

@seasoftjapan キー重複での INSERT は置いておいて、 DELETE の行を count() で確認すること自体に害は無さそうですが、いかがでしょうか？

[seasoftjapan]

@nanasess PostgreSQLが対象外となれば、当座黙認できる認識です。

MySQLは、後々時間ある時にでも手がけるかもしれませんが、別チケットでOKです。

[nanasess]

@seasoftjapan DELETE の前に、 DELETE と同じ WHERE の条件で COUNT をコールすると、 PostgreSQL でどんな悪影響が考えられますでしょうか？

[seasoftjapan]

@nanasess 無駄な実行の観点です。

[seasoftjapan]

あと、count() ではなく、exists() を使った方が良いですね。

[seasoftjapan]

@nobuhiko
| @seasoftjapan おっしゃっていることが全くわかりません。 INSERT は落ちませんよ。

想定シナリオが相違していたようです。
ご指摘ありがとうございます。

参照先記事のデッドロック例で双方のセッションが delete from lock_test where id = 100 だったため、registerShipping() に置き換えると $order_id が同一という想定で考えていました。通常の操作では発生しないと思うので、多重要求や決済システム絡みでWEBブラウザーとWEBフックが同時に動いたケースなど。

しかし、実際には $order_id は異なっていても生じ得ると理解いたしました。異なる場合には、INSERT はキー重複せず落ちないですね。

[nobuhiko]

@seasoftjapan
ちなみに空DELETEは無駄な実行ではないのでしょうか？

[nobuhiko]

@seasoftjapan
発生条件としては、新規受注で、registerShippingは必ず空DELETEを行うので受注が多く発生するサイトやバッチ処理で受注を作ったりするとネクストキーロックでdeadlockが発生し、あとからの受注が作られなくなります。

[seasoftjapan]

@seasoftjapan ちなみに空DELETEは無駄な実行ではないのでしょうか？

無駄ですね。しかし、SELECT するのとコストに差は無い認識でもあります。
よって、空 DELETE の場合はコストに大差はなく、実際に削除が必要な場合のコストが増えた認識です。

[seasoftjapan]

別の観点で、トランザクションを開いていない場合、MySQL でも無駄となりそうです。

ここまでに挙げた課題について、改善できそうな部分は追って PR しようと思います。

[seasoftjapan]

#1097 のコメントで、同一インスタンスで setOrder() など → select() など → delete() の流れで実行するとコケる問題が上がりましたが、#1116 で回避を見込んでいます。

[seasoftjapan]

備忘録: PostgreSQL では、REPEATABLE READ でも SERIALIZABLE でも、このケースのデッドロックは発生しなかった。

[seasoftjapan]

備忘録: 後続の改善案。今のところ、直ぐやる必要はなく、別チケットで良いと考える。

• 現在の分離レベルを変数に持たせる。
• 現在の分離レベルは、実装済みの DBMS 固定ごとのソース直書きに加えて、設定により以下のいずれかも選択可能とする。
  • 頭一発で現在のリアルな分離レベルを取得する。
  • ソース記載の定数値より優先して、分離レベルを指定する。
• 分離レベルの変更に対応する。
  • 設定に基づき頭一発実行だけなら簡単そう。途中変更にも対応すると、相手方への影響も考慮が必要かも (デッドロックでなく通常のロック待ちで問題ないのかも)。
• exists() を実行すべきかの判定結果 (若しくは判定条件の一部) は、キャッシュできそう。ただ、一般的な運用の範囲では、効果はかなり薄そう。