Fix rollback after statement fail in non-autocommit transaction #23247

hantangwangd · 2024-07-18T05:09:44Z

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 AbstractTestDistributedQueries to simulate the scenarios described in Session is completely corrupted by the failed statement in a non-autocommit transaction #23246
Manually confirm the fix in CLI console

Contributor checklist

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

Release Notes

== RELEASE NOTES ==

General Changes
* Fix ROLLBACK statement to ensure it successfully abort non-auto commit transactions corrupted by failed statements

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

elharo · 2024-07-18T11:31:52Z

presto-spark-base/src/main/java/com/facebook/presto/spark/PrestoSparkQueryExecutionFactory.java

 import static com.facebook.presto.spark.util.PrestoSparkUtils.getActionResultWithTimeout;
 import static com.facebook.presto.spi.StandardErrorCode.GENERIC_INTERNAL_ERROR;
 import static com.facebook.presto.spi.StandardErrorCode.INVALID_SESSION_PROPERTY;
+import static com.facebook.presto.sql.QueryUtil.isRollBack;


This couples to a new package & module. Probably not worth it for the trivial method you're importing.

Thanks for the comment, I also struggled here for a while. Fixed as your suggestion.

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java

tdcmeehan

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

hantangwangd · 2024-08-24T03:55:07Z

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?

elharo · 2024-09-23T18:00:49Z

Can you resolve the conflicts?

hantangwangd · 2024-09-23T19:30:22Z

Can you resolve the conflicts?

Sure, the conflicts are resolved!

tdcmeehan · 2024-11-04T16:47:16Z

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

    @Override
    public void checkCanSetRole(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, String role, String catalogName)
    {
        SemiTransactionalHiveMetastore metastore = getMetastore(transaction);


Can you help me understand why we need to check for null as part of this bug fix?

When a statement in a non-autocommit transaction fails, InMemoryTransactionManager.abortInternal() is triggered, which causes the relevant hive connector transaction manager to clean up it's corresponding connector transaction. However, the transaction in InMemoryTransactionManager is just marked as failed, but not removed, referring to https://github.com/prestodb/presto/blob/master/presto-main/src/main/java/com/facebook/presto/execution/QueryStateMachine.java#L918.

Therefore, if we do not check the null value here, an NPE exception will be encountered when executing the rollback statement.

If it's possible for metastore to be null, then shouldn't we check everywhere that we call metastore? and also label it as @Nullable?

justrelax19 · 2025-03-27T07:53:53Z

@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.

steveburnett · 2025-03-27T15:28:13Z

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

hantangwangd · 2025-03-27T16:09:17Z

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

ZacBlanco

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

ZacBlanco · 2025-04-09T22:55:49Z

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

    @Override
    public void checkCanSetRole(ConnectorTransactionHandle transaction, ConnectorIdentity identity, AccessControlContext context, String role, String catalogName)
    {
        SemiTransactionalHiveMetastore metastore = getMetastore(transaction);


If it's possible for metastore to be null, then shouldn't we check everywhere that we call metastore? and also label it as @Nullable?

ZacBlanco · 2025-04-09T22:57:17Z

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

    {
        TransactionalMetadata metadata = hiveTransactionManager.get(transaction);
-        return metadata.getMetastore();
+        return metadata == null ? null : metadata.getMetastore();


I think we should annotate this method with @Nullable if we are making the return behavior like this, or return Optional<SemiTransactionalHiveMetastore>

Good suggestion, I have changed the method to return an Optional<SemiTransactionalHiveMetastore>, and check everywhere we call getMetastore(transaction). Please take a look when convenient!

hantangwangd · 2025-04-10T11:45:41Z

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.

presto-main-base/src/main/java/com/facebook/presto/dispatcher/DispatchManager.java

ZacBlanco

Two minor things, otherwise LGTM

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java

ZacBlanco · 2025-04-22T05:31:48Z

presto-tests/src/main/java/com/facebook/presto/tests/AbstractTestDistributedQueries.java

+            assertUpdate("drop table if exists test_non_autocommit_table");
+        }
+        catch (Exception e) {
+            // ignored for connector compatibility


What does "for connector compatibility imply" here? Do some connectors behave differently?

Nice catch! Originally, I put the test case into AbstractTestIntegrationSmokeTest, and then found that its subclass TestJdbcIntegrationSmokeTest doesn't support table deletion, so I added this logic for compatibility.

Then I moved this test case to AbstractTestDistributedQueries, since there are much more subclasses of AbstractTestIntegrationSmokeTest that should ignore this test case because they don't support creating tables.

After recheck the subclasses, I found there is no need to add this logic for compatibility in AbstractTestDistributedQueries. So now it's removed again!

ZacBlanco

Thanks for this fix! LGTM

hantangwangd requested review from a team, ZacBlanco and shrinidhijoshi as code owners July 18, 2024 05:09

hantangwangd requested review from presto-oss and tdcmeehan July 18, 2024 05:09

elharo reviewed Jul 18, 2024

View reviewed changes

tdcmeehan self-assigned this Jul 18, 2024

hantangwangd force-pushed the fix_transaction branch from 3617d8d to c52de8c Compare July 18, 2024 15:10

ZacBlanco reviewed Jul 18, 2024

View reviewed changes

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Outdated Show resolved Hide resolved

ZacBlanco reviewed Jul 18, 2024

View reviewed changes

presto-hive/src/test/java/com/facebook/presto/hive/TestHiveIntegrationSmokeTest.java Outdated Show resolved Hide resolved

hantangwangd force-pushed the fix_transaction branch from c52de8c to 6107623 Compare July 18, 2024 16:46

hantangwangd force-pushed the fix_transaction branch from 6107623 to c5e36a2 Compare July 26, 2024 17:49

hantangwangd force-pushed the fix_transaction branch from c5e36a2 to 036171d Compare August 19, 2024 19:08

tdcmeehan reviewed Aug 23, 2024

View reviewed changes

hantangwangd force-pushed the fix_transaction branch from 036171d to 929a1d5 Compare September 9, 2024 01:29

hantangwangd force-pushed the fix_transaction branch from 929a1d5 to 58ff869 Compare September 23, 2024 19:29

hantangwangd force-pushed the fix_transaction branch from 58ff869 to b622a94 Compare September 29, 2024 05:18

hantangwangd force-pushed the fix_transaction branch from b622a94 to 6903d3e Compare October 7, 2024 16:12

hantangwangd force-pushed the fix_transaction branch from 6903d3e to ac9fd7e Compare November 3, 2024 13:49

tdcmeehan reviewed Nov 4, 2024

View reviewed changes

hantangwangd force-pushed the fix_transaction branch from ac9fd7e to 73b0a76 Compare March 27, 2025 17:37

jaystarshot requested a review from ZacBlanco April 2, 2025 23:19

ZacBlanco reviewed Apr 9, 2025

View reviewed changes

hantangwangd force-pushed the fix_transaction branch from 73b0a76 to 0a51e30 Compare April 10, 2025 10:26

ZacBlanco requested changes Apr 14, 2025

View reviewed changes

presto-main-base/src/main/java/com/facebook/presto/dispatcher/DispatchManager.java Outdated Show resolved Hide resolved

hantangwangd force-pushed the fix_transaction branch 2 times, most recently from 72abe0f to 55570d8 Compare April 21, 2025 10:20

ZacBlanco requested changes Apr 21, 2025

View reviewed changes

presto-hive/src/main/java/com/facebook/presto/hive/security/SqlStandardAccessControl.java Outdated Show resolved Hide resolved

presto-iceberg/src/test/java/com/facebook/presto/iceberg/IcebergDistributedSmokeTestBase.java Outdated Show resolved Hide resolved

hantangwangd requested review from ClarenceThreepwood, feilong-liu and jaystarshot as code owners April 22, 2025 03:39

ZacBlanco reviewed Apr 22, 2025

View reviewed changes

hantangwangd force-pushed the fix_transaction branch from a295c86 to 9c7359e Compare April 22, 2025 06:32

ZacBlanco approved these changes Apr 22, 2025

View reviewed changes

Fix rollback after statement fail in non-autocommit transaction

9d25512

hantangwangd force-pushed the fix_transaction branch from 9c7359e to 9d25512 Compare April 22, 2025 16:53

hantangwangd merged commit 34ef2c7 into prestodb:master Apr 22, 2025
98 checks passed

hantangwangd deleted the fix_transaction branch April 22, 2025 23:12

hantangwangd mentioned this pull request Apr 23, 2025

Fix commit after statement fail in non-autocommit transaction #24960

Open

6 tasks

hantangwangd mentioned this pull request May 27, 2025

After a Presto transaction is rollback successfully, new insert operations will fail #25204

Closed

ZacBlanco mentioned this pull request May 29, 2025

Add release notes for 0.293 #25222

Merged

21 tasks

This was referenced May 30, 2025

Add release notes for 0.293 unix280/presto#31

Closed

Add release notes for 0.293 unix280/presto#32

Closed

Fix rollback after statement fail in non-autocommit transaction #23247

Fix rollback after statement fail in non-autocommit transaction #23247

Uh oh!

Conversation

hantangwangd commented Jul 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation

Test Plan

Contributor checklist

Release Notes

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tdcmeehan left a comment

Choose a reason for hiding this comment

Uh oh!

hantangwangd commented Aug 24, 2024

Uh oh!

elharo commented Sep 23, 2024

Uh oh!

hantangwangd commented Sep 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

justrelax19 commented Mar 27, 2025

Uh oh!

steveburnett commented Mar 27, 2025

Uh oh!

hantangwangd commented Mar 27, 2025

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hantangwangd commented Apr 10, 2025

Uh oh!

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ZacBlanco left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hantangwangd commented Jul 18, 2024 •

edited

Loading