Skip to content

fix(bigquery-jdbc): avoid rollback on statement close in manual commit mode#13503

Open
yagudin10 wants to merge 4 commits into
googleapis:mainfrom
yagudin10:main
Open

fix(bigquery-jdbc): avoid rollback on statement close in manual commit mode#13503
yagudin10 wants to merge 4 commits into
googleapis:mainfrom
yagudin10:main

Conversation

@yagudin10

Copy link
Copy Markdown
Contributor

Problem

Manual commit mode does not work correctly because statement cancellation triggers a transaction rollback.

STR (Steps to Reproduce)

  1. Create a JDBC connection.

  2. Disable auto-commit:

    conn.setAutoCommit(false);
  3. Execute a DML statement using a PreparedStatement.

  4. Close the statement.

  5. Call:

    conn.commit();

ER (Expected Result)

  • Closing a statement does not affect the active transaction.
  • Pending changes remain available until an explicit commit() or rollback() is performed on the connection.
  • conn.commit() successfully persists transaction changes.

AR (Actual Result)

  • Statement cleanup invokes cancel().
  • cancel() performs a transaction rollback.
  • Pending transaction changes are discarded before conn.commit() is called.
  • conn.commit() has no effect because the transaction has already been rolled back.

Sample Java Code:

try (Connection conn = DriverManager.getConnection(jdbcUrl, props)) {
    conn.setAutoCommit(false);

    try (PreparedStatement ps =
             conn.prepareStatement(
                 "INSERT INTO test_table(id) VALUES (1)")) {
        ps.executeUpdate();
    } // statement cleanup invokes cancel() -> rollback

    conn.commit(); // nothing is committed
}

Fix

Remove transaction rollback from the statement cancellation path. Transaction state is now managed exclusively through explicit Connection.commit() and Connection.rollback() operations.

@yagudin10 yagudin10 requested review from a team as code owners June 17, 2026 08:02

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request removes the automatic transaction rollback when closing statement resources in BigQueryStatement and adds unit and integration tests to verify that canceling or closing statements does not roll back active transactions. The review feedback points out potential resource leaks in the newly added integration tests where connections, prepared statements, and result sets are not safely closed in case of exceptions, and suggests refactoring them to use try-with-resources.

Comment on lines +2341 to +2375
Connection connection = DriverManager.getConnection(session_enabled_connection_uri);
connection.setAutoCommit(false);

PreparedStatement ps1 = connection.prepareStatement(insertQuery);
PreparedStatement ps2 = connection.prepareStatement(insertQuery);
try {
ps1.setInt(1, 1);
ps1.setString(2, "DwightShrute");
ps1.setInt(3, 10);
assertEquals(1, ps1.executeUpdate());

ps2.setInt(1, 2);
ps2.setString(2, "MichaelScott");
ps2.setInt(3, 20);
assertEquals(1, ps2.executeUpdate());

ps1.close();
connection.commit();

ResultSet resultSet = bigQueryStatement.executeQuery(selectQuery);
int rowCount = 0;
while (resultSet.next()) {
rowCount++;
assertEquals(rowCount, resultSet.getInt(1));
}
assertEquals(2, rowCount);
} finally {
try {
ps2.close();
} finally {
bigQueryStatement.execute(
String.format("DROP TABLE IF EXISTS %s.%s", DATASET, TRANSACTION_TABLE));
connection.close();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The connection, prepared statements, and result set are not safely closed if an exception occurs during test execution. If connection.prepareStatement or any other statement throws an exception, the database connection and statements will be leaked. Using try-with-resources ensures that all closeable resources are properly released in all execution paths.

    try (Connection connection = DriverManager.getConnection(session_enabled_connection_uri)) {
      connection.setAutoCommit(false);
      try (PreparedStatement ps1 = connection.prepareStatement(insertQuery);
           PreparedStatement ps2 = connection.prepareStatement(insertQuery)) {
        ps1.setInt(1, 1);
        ps1.setString(2, "DwightShrute");
        ps1.setInt(3, 10);
        assertEquals(1, ps1.executeUpdate());

        ps2.setInt(1, 2);
        ps2.setString(2, "MichaelScott");
        ps2.setInt(3, 20);
        assertEquals(1, ps2.executeUpdate());

        ps1.close();
        connection.commit();

        try (ResultSet resultSet = bigQueryStatement.executeQuery(selectQuery)) {
          int rowCount = 0;
          while (resultSet.next()) {
            rowCount++;
            assertEquals(rowCount, resultSet.getInt(1));
          }
          assertEquals(2, rowCount);
        }
      }
    } finally {
      bigQueryStatement.execute(
          String.format("DROP TABLE IF EXISTS %s.%s", DATASET, TRANSACTION_TABLE));
    }
References
  1. When managing a collection of closeable resources (e.g., scopes), ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

Comment on lines +2393 to +2422
Connection connection = DriverManager.getConnection(session_enabled_connection_uri);
connection.setAutoCommit(false);

PreparedStatement ps1 = connection.prepareStatement(insertQuery);
PreparedStatement ps2 = connection.prepareStatement(insertQuery);
try {

ps2.setInt(1, 1);
ps2.setString(2, "MichaelScott");
ps2.setInt(3, 20);
assertEquals(1, ps2.executeUpdate());

ps1.close();
connection.commit();

ResultSet resultSet = bigQueryStatement.executeQuery(selectQuery);
assertTrue(resultSet.next());
assertEquals(1, resultSet.getInt(1));
assertEquals("MichaelScott", resultSet.getString(2));
assertEquals(20, resultSet.getInt(3));
assertFalse(resultSet.next());
} finally {
try {
ps2.close();
} finally {
bigQueryStatement.execute(
String.format("DROP TABLE IF EXISTS %s.%s", DATASET, TRANSACTION_TABLE));
connection.close();
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Similarly, the connection, prepared statements, and result set in this test are not safely closed in case of exceptions, leading to potential resource leaks. Refactoring to use try-with-resources ensures robust resource management.

    try (Connection connection = DriverManager.getConnection(session_enabled_connection_uri)) {
      connection.setAutoCommit(false);
      try (PreparedStatement ps1 = connection.prepareStatement(insertQuery);
           PreparedStatement ps2 = connection.prepareStatement(insertQuery)) {

        ps2.setInt(1, 1);
        ps2.setString(2, "MichaelScott");
        ps2.setInt(3, 20);
        assertEquals(1, ps2.executeUpdate());

        ps1.close();
        connection.commit();

        try (ResultSet resultSet = bigQueryStatement.executeQuery(selectQuery)) {
          assertTrue(resultSet.next());
          assertEquals(1, resultSet.getInt(1));
          assertEquals("MichaelScott", resultSet.getString(2));
          assertEquals(20, resultSet.getInt(3));
          assertFalse(resultSet.next());
        }
      }
    } finally {
      bigQueryStatement.execute(
          String.format("DROP TABLE IF EXISTS %s.%s", DATASET, TRANSACTION_TABLE));
    }
References
  1. When managing a collection of closeable resources (e.g., scopes), ensure they are closed in the reverse order of their creation (LIFO). The implementation must be exception-safe to prevent resource leaks, meaning all opened resources should be closed even if exceptions occur during their creation or closing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please fix; I think using try-with-resources would address the issue

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed

this.currentUpdateCount = -1;
this.currentJobIdIndex = -1;
if (this.connection != null) {
if (this.connection.isTransactionStarted()) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you please add this logic to the connection.closeImpl()?

I think when connection is closed, it needs to cancel any pending transaction otherwise they will be eventually cancelled by some timeout.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.
Not sure about calling rollbackImpl() as-is, since it starts a new transaction after the rollback, which seems unnecessary during close().
On the other hand, connection.close() may fail before the connection is fully closed, so keeping the existing rollback behavior might actually be the safer option.

@yagudin10 yagudin10 requested a review from logachev June 18, 2026 09:38
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