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

beginTransaction() ではなく setAutoCommit(false) を使用する #1518

Closed
nanasess opened this Issue Mar 10, 2016 · 12 comments

Comments

Projects
None yet
6 participants
@nanasess
Contributor

nanasess commented Mar 10, 2016

現在、 Connection::beginTransaction() が使用されています。
この関数は、ネストしたトランザクションブロックを作ることができます。
この実装は、 savepoint を発行するのみなので、以下のようなケースで EXECUTE SQL 2 が ROLLBACK すると、 1 と 3 はコミットされてしまいます。

BEGIN 1
    EXECUTE SQL 1
    BEGIN 2
        EXECUTE SQL 2
    COMMIT 2
    EXECUTE SQL 3
COMMIT

Connection::setAutoCommit(false) を使用すれば、 commit() をコールした時点で新たなトランザクションが開始されますので、 commit() 時点までのロールバックが可能です。
プラグインの使用を考えると、 setAutoCommit(false) を使用し、更新を反映させたい時点で commit() という実装の方が直感的かつ安全かと思いますが、いかがでしょうか。

@seasoftjapan

This comment has been minimized.

Show comment
Hide comment
@seasoftjapan

seasoftjapan Mar 10, 2016

Contributor

以下のようなケースで EXECUTE SQL 2 が ROLLBACK すると、 1 と 3 はコミットされてしまいます。

それは、最終行で COMMIT しているからですよね?

ならば、その動作の方が自然な気がしました。

現況の例外処理がどうなっているのか把握していませんが、例外処理に回った場合に、全体がコミットされなければ、それで十分なように思います。

Contributor

seasoftjapan commented Mar 10, 2016

以下のようなケースで EXECUTE SQL 2 が ROLLBACK すると、 1 と 3 はコミットされてしまいます。

それは、最終行で COMMIT しているからですよね?

ならば、その動作の方が自然な気がしました。

現況の例外処理がどうなっているのか把握していませんが、例外処理に回った場合に、全体がコミットされなければ、それで十分なように思います。

@nanasess

This comment has been minimized.

Show comment
Hide comment
@nanasess

nanasess Mar 10, 2016

Contributor

@seasoftjapan 2 の BEGIN/COMMIT が、実は savepoint だというのを、実装者が意識できていれば問題ないと思いますが、そうでなければ commit() の時点まで の方が、安全で、わかりやすいかなと思いました。
特に、プラグインの中で commit() とか rollback() とかやられると、よくわからなくなるので...

Contributor

nanasess commented Mar 10, 2016

@seasoftjapan 2 の BEGIN/COMMIT が、実は savepoint だというのを、実装者が意識できていれば問題ないと思いますが、そうでなければ commit() の時点まで の方が、安全で、わかりやすいかなと思いました。
特に、プラグインの中で commit() とか rollback() とかやられると、よくわからなくなるので...

@ttsuru

This comment has been minimized.

Show comment
Hide comment
@ttsuru

ttsuru Mar 10, 2016

Contributor

@nanasess
入れ子になった際に不思議な感じになりませんかね・・・

Contributor

ttsuru commented Mar 10, 2016

@nanasess
入れ子になった際に不思議な感じになりませんかね・・・

@ttsuru

This comment has been minimized.

Show comment
Hide comment
@ttsuru

ttsuru Mar 10, 2016

Contributor

プラグインを考えると、commit以前にフックポイントがある事のほうが大事な気がしますがどうでしょうか。

Contributor

ttsuru commented Mar 10, 2016

プラグインを考えると、commit以前にフックポイントがある事のほうが大事な気がしますがどうでしょうか。

@nanasess

This comment has been minimized.

Show comment
Hide comment
@nanasess

nanasess Mar 10, 2016

Contributor

@ttsuru ちゃんと入れ子は savepoint だと意識して、 入れ子の中で beginTransaction() / commit() していれば、入れ子のみロールバックするので問題ないと思いますよ。
プラグインの中で beginTransaction() のみコールされると困ったことになりますが...

Contributor

nanasess commented Mar 10, 2016

@ttsuru ちゃんと入れ子は savepoint だと意識して、 入れ子の中で beginTransaction() / commit() していれば、入れ子のみロールバックするので問題ないと思いますよ。
プラグインの中で beginTransaction() のみコールされると困ったことになりますが...

@ttsuru

This comment has been minimized.

Show comment
Hide comment
@ttsuru

ttsuru Mar 10, 2016

Contributor

@nanasess
それであれば、 Connection::beginTransaction() でいいのではないでしょうか。
refs のように setAutoCommit(false) は挙動が異なります。

savepoint という概念もここで登場することはわかりづらいと思います。

Contributor

ttsuru commented Mar 10, 2016

@nanasess
それであれば、 Connection::beginTransaction() でいいのではないでしょうか。
refs のように setAutoCommit(false) は挙動が異なります。

savepoint という概念もここで登場することはわかりづらいと思います。

@shhirose

This comment has been minimized.

Show comment
Hide comment
@shhirose

shhirose Mar 10, 2016

Contributor

Transaction を入れ子にしなければならないケースは稀ではないでしょうか?(何かしらの処理を行った記録を残す場合など?)
入力の項目追加などはプラグインで失敗した場合は、本体側もロールバックしてもらいたいと思います。

であれば、フックポイントの名称に Transaction が開始しているかどうかを判断できるような形にして、その後は開発者に委ねてしまって良い気がします。ただし、プラグインの開発手順にネストする場合はtry-cache入れるとか、commitrollbackを確実に行うなどの記述は必要だとは思いますが。

Contributor

shhirose commented Mar 10, 2016

Transaction を入れ子にしなければならないケースは稀ではないでしょうか?(何かしらの処理を行った記録を残す場合など?)
入力の項目追加などはプラグインで失敗した場合は、本体側もロールバックしてもらいたいと思います。

であれば、フックポイントの名称に Transaction が開始しているかどうかを判断できるような形にして、その後は開発者に委ねてしまって良い気がします。ただし、プラグインの開発手順にネストする場合はtry-cache入れるとか、commitrollbackを確実に行うなどの記述は必要だとは思いますが。

@nanasess

This comment has been minimized.

Show comment
Hide comment
@nanasess

nanasess Mar 10, 2016

Contributor

入力の項目追加などはプラグインで失敗した場合は、本体側もロールバックしてもらいたいと思います。

これを確実に実現するためには、 Controller 全体をトランザクションブロックで囲まねばならず、そうした時に setAutoCommit(false) の方が挙動が把握しやすく、安全ではないかと思ったのでした。

現在は問題が具現化しておらず、 setAutoCommit(false) にするメリットもわかりづらいと思いますので、一度再現ケースを作ってみようと思います。

Contributor

nanasess commented Mar 10, 2016

入力の項目追加などはプラグインで失敗した場合は、本体側もロールバックしてもらいたいと思います。

これを確実に実現するためには、 Controller 全体をトランザクションブロックで囲まねばならず、そうした時に setAutoCommit(false) の方が挙動が把握しやすく、安全ではないかと思ったのでした。

現在は問題が具現化しておらず、 setAutoCommit(false) にするメリットもわかりづらいと思いますので、一度再現ケースを作ってみようと思います。

@ttsuru

This comment has been minimized.

Show comment
Hide comment
@ttsuru

ttsuru Mar 12, 2016

Contributor

setAutoCommit(false) をしてしまうと、作法の悪いプラグインに勝手にcommitされてしまう気がします。

僕も @shhirose さんと以下の内容に同意見です。

入力の項目追加などはプラグインで失敗した場合は、本体側もロールバックしてもらいたいと思います。

また、本件が最も重要になるのは注文だと思いますが、現状改善すべきユースケースがプラグインの記載含め見当たりません。
逆にtry catch のcatchにhookpointが欲しいぐらいでしょうか。

Contributor

ttsuru commented Mar 12, 2016

setAutoCommit(false) をしてしまうと、作法の悪いプラグインに勝手にcommitされてしまう気がします。

僕も @shhirose さんと以下の内容に同意見です。

入力の項目追加などはプラグインで失敗した場合は、本体側もロールバックしてもらいたいと思います。

また、本件が最も重要になるのは注文だと思いますが、現状改善すべきユースケースがプラグインの記載含め見当たりません。
逆にtry catch のcatchにhookpointが欲しいぐらいでしょうか。

@nanasess nanasess added the bug label Jul 12, 2016

@nanasess nanasess added this to the 3.0.11 milestone Jul 12, 2016

@chihiro-adachi

This comment has been minimized.

Show comment
Hide comment
@chihiro-adachi

chihiro-adachi Jul 25, 2016

Contributor

いくつかユースケースも合わせて検証してみました。

master...chihiro-adachi:dev-autocommit

アプリケーションの開始/終了でコントロールするようにしています。
プラグイン側でシステムエラー等が発生した場合は、本体側も含めロールバックを行います。

  • Requestイベントでbegin
  • Terminateイベントでcommit
  • 例外発生時やcommitできない場合はrollback

のような実装です。

BEGIN 1
    EXECUTE SQL 1
    BEGIN 2
        EXECUTE SQL 2
    COMMIT 2
    EXECUTE SQL 3
COMMIT

のユースケース例だと、

  • EXECUTE SQL 2 で 例外が発生した場合:EXECUTE SQL 1/2はROLLBACKされる
  • COMMIT2が正常に終了し、EXECUTE SQL 3の前に例外が発生した場合:ECUTE SQL 1/ 2 はROLLBACKされる
  • COMMIT2が正常に終了し、EXECUTE SQL 3の後に例外が発生した場合:ECUTE SQL 1/ 2 /3 はROLLBACKされる

となります。

トランザクションは本体側の制御にまかせ、プラグイン側では扱わないようにしてもらえれば、本体側で一貫した制御が出来るかと思います。

Contributor

chihiro-adachi commented Jul 25, 2016

いくつかユースケースも合わせて検証してみました。

master...chihiro-adachi:dev-autocommit

アプリケーションの開始/終了でコントロールするようにしています。
プラグイン側でシステムエラー等が発生した場合は、本体側も含めロールバックを行います。

  • Requestイベントでbegin
  • Terminateイベントでcommit
  • 例外発生時やcommitできない場合はrollback

のような実装です。

BEGIN 1
    EXECUTE SQL 1
    BEGIN 2
        EXECUTE SQL 2
    COMMIT 2
    EXECUTE SQL 3
COMMIT

のユースケース例だと、

  • EXECUTE SQL 2 で 例外が発生した場合:EXECUTE SQL 1/2はROLLBACKされる
  • COMMIT2が正常に終了し、EXECUTE SQL 3の前に例外が発生した場合:ECUTE SQL 1/ 2 はROLLBACKされる
  • COMMIT2が正常に終了し、EXECUTE SQL 3の後に例外が発生した場合:ECUTE SQL 1/ 2 /3 はROLLBACKされる

となります。

トランザクションは本体側の制御にまかせ、プラグイン側では扱わないようにしてもらえれば、本体側で一貫した制御が出来るかと思います。

@ryo-endo

This comment has been minimized.

Show comment
Hide comment
@ryo-endo

ryo-endo Aug 17, 2016

Contributor

#1632
こちらの内容、PR取り込みましたのでIssueを閉じます。

Contributor

ryo-endo commented Aug 17, 2016

#1632
こちらの内容、PR取り込みましたのでIssueを閉じます。

@ryo-endo ryo-endo closed this Aug 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment