presto icon indicating copy to clipboard operation
presto copied to clipboard

Fix rollback after statement fail in non-autocommit transaction

Open hantangwangd opened this issue 1 year ago • 3 comments

Description

This PR fix the session corruption cause by a failed statement in non-autocommit transaction, enable executing rollback to end the aborted transaction block.

Motivation

Fix: #23246

Test Plan

  • Newly added test cases in hive and iceberg to simulate the scenarios described in #23246
  • Manually confirm the fix in CLI console

Contributor checklist

  • [x] Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • [x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • [x] Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • [x] If release notes are required, they follow the release notes guidelines.
  • [x] Adequate tests were added if applicable.
  • [x] CI passed.

Release Notes

== NO RELEASE NOTE ==

hantangwangd avatar Jul 18 '24 05:07 hantangwangd

It seems like ignoreTransactionState is more like isRollingBack. Do you think that's an accurate way of describing it?

My thought is, we may want to support other statements in the future for this situation (such as commit, although the commit result should fail, but it should be able to close the entire transaction too). So name this flag ignoreTransactionState rather than isRollback, do you think it makes sense?

hantangwangd avatar Aug 24 '24 03:08 hantangwangd

Can you resolve the conflicts?

elharo avatar Sep 23 '24 18:09 elharo

Can you resolve the conflicts?

Sure, the conflicts are resolved!

hantangwangd avatar Sep 23 '24 19:09 hantangwangd

@tdcmeehan and @hantangwangd any update on the above PR since its been open for about 5 months now and if we can merge and close it ? Because we are also receiving the aborted transaction block issue and seems like the above PR will fix it.

justrelax19 avatar Mar 27 '25 07:03 justrelax19

@hantangwangd, to move this one towards merge-ready, when you have time would you rebase and resolve the file conflicts?

steveburnett avatar Mar 27 '25 15:03 steveburnett

Sure, I will rebase and resolve the conflicts later this day. @steveburnett @justrelax19

hantangwangd avatar Mar 27 '25 16:03 hantangwangd

I mostly agree with Tim's thought from his comment here

But I also understand your reasoning in the response. Can we add a well-written javadoc which explains the meaning and use of this field so that it's clearer to developers the intention of this name? isRollback seems clear to me, but isIgnoreTransactionState does not

Thanks for the feedback, I agree with you that isIgnoreTransactionState might bring some confusions to future developers. To make its intention more clearer, and to make it applicable to some other statements (for example, maybe commit in the future) as well, do you think it makes sense to call it enableRollback? I'm open on this, and do not have a strong inclination, please let me know your thought.

hantangwangd avatar Apr 10 '25 11:04 hantangwangd