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

Fix failure with enabling JDBC autocommit #23453

Merged
merged 1 commit into from
Aug 16, 2024

Conversation

rschlussel
Copy link
Contributor

@rschlussel rschlussel commented Aug 15, 2024

Description

There was a bug where enabling autocommit when it had previously been false would cause failure like:
java.sql.SQLException: Connection is in auto-commit mode

This was due to calling commit() from setAutoCommit() after autocommit had already been changed to true.

Motivation and Context

Fixes #15916

Impact

Fixes a failure when using JDBC when setting AutoCommit from false to true

Test Plan

unit test

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

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

JDBC Changes
* Fix failure when setting autoCommit from false to true :pr:`23453`

@steveburnett
Copy link
Contributor

Just a nit to add the PR number to release note entry.

== RELEASE NOTES ==

JDBC Changes
* Fix failure when setting autoCommit from false to true :pr:`23453`

connection.setAutoCommit(false);
try (Statement statement = connection.createStatement()) {
statement.execute("CREATE TABLE test_commit (x bigint)");
try {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to see in github, but the only thing that's changed about this test is that it's wrapped in an extra try so I could put a finally at the end to drop the table.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A neat trick is to take the url in the browser and add the query parameter ?w=1, which will cause whitespaces to be ignored. e.g. https://github.com/prestodb/presto/pull/23453/files?w=1

@rschlussel rschlussel marked this pull request as ready for review August 15, 2024 19:38
@rschlussel rschlussel requested a review from a team as a code owner August 15, 2024 19:38
tdcmeehan
tdcmeehan previously approved these changes Aug 15, 2024
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one little nit.

There was a bug where enabling autocommit when it had previously been
false would cause failure like:
java.sql.SQLException: Connection is in auto-commit mode

This was due to calling commit() from setAutoCommit() after autocommit
had already been changed to true.

Fixes prestodb#15916
Copy link
Member

@hantangwangd hantangwangd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix.

@tdcmeehan tdcmeehan merged commit bd444ec into prestodb:master Aug 16, 2024
56 checks passed
@tdcmeehan tdcmeehan mentioned this pull request Aug 23, 2024
34 tasks
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.

SQLException thrown when Connection.setAutoCommit(false) and then Connection.setAutoCommit(false);
5 participants