Make table metadata addition atomic#3464
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces manual transaction management within the TableMetadataService.addTableMetadata method to ensure that metadata updates are atomic. The changes include logic to disable auto-commit, perform metadata operations, and handle commits or rollbacks, while restoring the original auto-commit state in a finally block. Corresponding unit tests in JdbcAdminTest were updated to verify the commit operation. Feedback was provided regarding a potential issue where calling commit() could unintentionally finalize an external transaction if the connection was already in a non-auto-commit state.
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
Pull request overview
This PR aims to make JDBC table-metadata updates atomic by wrapping the delete+insert sequence in a single transaction, ensuring metadata isn’t left partially updated if an error occurs mid-operation.
Changes:
- Updated
TableMetadataService.addTableMetadata()to run metadata overwrite/insert operations within an explicit transaction and commit/rollback as a unit. - Adjusted
JdbcAdminTestexpectations to assert that a commit occurs after successful metadata additions.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| core/src/main/java/com/scalar/db/storage/jdbc/TableMetadataService.java | Introduces explicit transaction boundaries (auto-commit toggle, commit/rollback) around metadata writes to make them atomic. |
| core/src/test/java/com/scalar/db/storage/jdbc/JdbcAdminTest.java | Updates assertions to reflect the new commit behavior after adding table metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| connection.commit(); | ||
| } catch (SQLException e) { | ||
| try { | ||
| connection.rollback(); | ||
| } catch (SQLException rollbackEx) { |
There was a problem hiding this comment.
addTableMetadata() unconditionally calls connection.commit() / connection.rollback() even when the caller already provided a connection with autoCommit=false (i.e., an existing transaction). That can unexpectedly commit/rollback the caller’s broader transaction and can also lead to redundant rollbacks when this method is invoked inside JdbcAdmin.withConnection() (which also rolls back on exceptions when requiresExplicitCommit is true). Consider committing/rolling back only when this method started the transaction (e.g., when originalAutoCommit was true), and when originalAutoCommit is false use a savepoint (rollback-to-savepoint on error) or leave commit/rollback to the caller.
There was a problem hiding this comment.
In the current codebase, there are no uncommitted changes on the connection when addTableMetadata() is called. All prior operations within withConnection() lambda already commit individually via execute(connection, sql, requiresExplicitCommit). So the unconditional commit() here does not accidentally finalize someone else's work.
The redundant rollback (both addTableMetadata and withConnection rolling back on error) is harmless.
I want to keep the change scoped to addTableMetadata only, without modifying withConnection(), which is widely used across JdbcAdmin.
@brfrn169 What do you think?
KodaiD
requested review from
a team,
Torch3333,
brfrn169,
feeblefakie and
komamitsu
and removed request for
a team
feeblefakie
left a comment
feeblefakie
left a comment
There was a problem hiding this comment.
LGTM! Thank you!
Left one minor suggestion, so PTAL!
| if (originalAutoCommit) { | ||
| connection.setAutoCommit(false); | ||
| } |
There was a problem hiding this comment.
It's minor, but can we just do as follows if we always avoid auto-commit?
| if (originalAutoCommit) { | |
| connection.setAutoCommit(false); | |
| } | |
| connection.setAutoCommit(false); |
| execute( | ||
| connection, getDeleteTableMetadataStatement(namespace, table), requiresExplicitCommit); |
There was a problem hiding this comment.
requiresExplicitCommit is false except when using Oracle with the SERIALIZABLE isolation level. So this change handles the Oracle + SERIALIZABLE case, correct?
(Update) Sorry, I was wrong here. Please disregard this comment.,
|
@brfrn169 and I talked it over, and we've decided to look into a more radical refactor for this issue. We'll discuss this again post-v3.18 release. For now, I'll set this PR to Draft. |
Description
This PR makes metadata addition in DDLs more robust.
TableMetadataService.addTableMetadatapreviously committed each SQL statement (DELETE and per-column INSERTs) individually. If a failure occurred mid-way, the table metadata could be left in an incomplete state. We can userepairTablein such a case, but it would be more robust if these metadata writes were executed atomically. This PR wraps the DELETE and all INSERTs in a single transaction to ensure atomicity.Related issues and/or PRs
N/A
Changes made
TableMetadataService.addTableMetadatain a single transaction with explicit commit and rollback on failure.verify(connection).commit()assertions to existingTableMetadataService.addTableMetadataunit tests.Checklist
Additional notes (optional)
N/A
Release notes
Updated
Adminto make multiple metadata writes atomic.