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

環境変数設定時にdeadlockするのを修正 #890

Merged
merged 2 commits into from
Apr 2, 2024

Conversation

pirosiki197
Copy link
Contributor

なぜやるか

環境変数を一度に複数設定するとdeadlockする
deadlock-sample
何回か Save を押せば一応ちゃんと保存される

やったこと

application テーブルでロックをとるようにした

資料

https://zenn.dev/shuntagami/articles/ea44a20911b817

@pirosiki197 pirosiki197 requested a review from motoki317 April 2, 2024 12:11
@motoki317
Copy link
Member

根本はsqlboilerで複数unique columnでupsertできないからtransactionを使っているのであって、これが解決できればそちらを使うほうが良さそうですね...
(コメントにもある通り: https://github.com/volatiletech/sqlboiler/issues/328)

もしくは並列になってる複数リクエストを直列にするか、1つにまとめるように改善するかすると、まず起こることは圧倒的に少なくなりますね

await Promise.all([...addEnvVarRequests, ...deleteEnvVarRequests])

その記事読んだことあるけどgap lockの罠を忘れてました gap lockするのあんまりよくないですね
https://dev.mysql.com/doc/refman/8.3/en/innodb-locking.html#:~:text=It%20is%20also,must%20be%20merged.

方針としてはapplicationテーブルの方でロックするので良いと思います
(次から何か気づいたら、issueから立ててくれると議論などがやりやすいです!)

motoki317
motoki317 previously approved these changes Apr 2, 2024
pkg/infrastructure/repository/environment.go Outdated Show resolved Hide resolved
@pirosiki197 pirosiki197 merged commit 8cf2dfa into main Apr 2, 2024
14 checks passed
@pirosiki197 pirosiki197 deleted the fix/setenv-deadlock branch April 2, 2024 13:19
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.

2 participants