-
Notifications
You must be signed in to change notification settings - Fork 123
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
feat: savepoints #2278
feat: savepoints #2278
Conversation
Adds support for savepoints to the Connection API. Savepoints use the internal retry mechanism for read/write transactions to emulate actual savepoints. Rolling back to a savepoint is guaranteed to always work, but resuming the transaction after rolling back can fail if the underlying data has been modified, or if one or more of the statements before the savepoint that was rolled back to returns non-deterministic data.
} | ||
} | ||
|
||
private final LinkedList<Savepoint> savepoints = new LinkedList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine you already picked the best data structure here, but wondering if you considered any other like Deque
for stack like popping or a TreeMap
for fast retrieval of elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I considered a couple of different options, but landed on LinkedList
both for simplicity, but also because it aligns with how SAVEPOINT
works. They are strictly ordered by an index (i.e. a map structure makes less sense, as there is no mapping between a key and a value other than a simple index key), and removing a savepoint from the list always means removing everything after it as well. That means that cutting one node in the middle of the list is a cheap operation.
That being said; a transaction is unlikely to ever have more than a handful of savepoints, meaning that the chosen data structure won't have any practical effect on the performance.
Deque
was indeed one that I considered, but I dropped it as we need to be able to peek multiple steps back, which is not supported by a pureDeque
implementation.TreeMap
or similar would not really be a good fit, as the key in this case is just an index. The savepoint names also do not need to be unique (in PostgreSQL).
|
||
/** | ||
* Rolls back to the given savepoint. Rolling back to a savepoint undoes all changes and releases | ||
* all locks that have been taken after the savepoint. Rolling back to a savepoint does not remove |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean internal locks, as we don't allow for "SQL" locks in Cloud Spanner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
} | ||
assertEquals(RANDOM_RESULT_SET_ROW_COUNT, count); | ||
} | ||
connection.commit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if I commit after a rollbackToSavepoint? (I assume it would potentially work with the statements up to the given savepoint, if nothing has changed, but just verifying my understanding)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your understanding is correct. The idea is that a savepoint will allow you to reset the transaction to a valid point and then either continue or commit, instead of having to abort the entire transaction.
connection.executeUpdate(INSERT_STATEMENT); | ||
connection.executeUpdate(INSERT_STATEMENT); | ||
// Change the result of the initial query. | ||
mockSpanner.putStatementResult(StatementResult.query(statement, generator.generate())); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice test
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks :-)
Merge-on-green attempted to merge your PR for 6 hours, but it was not mergeable because either one of your required status checks failed, one of your required reviews was not approved, or there is a do not merge label. Learn more about your required status checks here: https://help.github.com/en/github/administering-a-repository/enabling-required-status-checks. You can remove and reapply the label to re-run the bot. |
🤖 I have created a release *beep* *boop* --- ## [6.40.0](https://togithub.com/googleapis/java-spanner/compare/v6.39.0...v6.40.0) (2023-04-14) ### Features * Savepoints ([#2278](https://togithub.com/googleapis/java-spanner/issues/2278)) ([b02f584](https://togithub.com/googleapis/java-spanner/commit/b02f58435b97346cc8e08a96635affe8383981bb)) ### Performance Improvements * Remove custom transport executor ([#2366](https://togithub.com/googleapis/java-spanner/issues/2366)) ([e27dbe5](https://togithub.com/googleapis/java-spanner/commit/e27dbe5f58229dab208eeeed44d53e741700c814)) ### Dependencies * Update dependency com.google.cloud:google-cloud-shared-dependencies to v3.7.0 ([#2377](https://togithub.com/googleapis/java-spanner/issues/2377)) ([40402af](https://togithub.com/googleapis/java-spanner/commit/40402af54f94f16619d018e252181db29ae6855e)) * Update dependency org.graalvm.buildtools:junit-platform-native to v0.9.21 ([#2379](https://togithub.com/googleapis/java-spanner/issues/2379)) ([ae7262d](https://togithub.com/googleapis/java-spanner/commit/ae7262d37391c0ec2fee1dcbb24899e4fa16ae17)) * Update dependency org.graalvm.buildtools:native-maven-plugin to v0.9.21 ([#2380](https://togithub.com/googleapis/java-spanner/issues/2380)) ([0cb159e](https://togithub.com/googleapis/java-spanner/commit/0cb159efc97f02b42f064244e3812a0fd3d82db6)) --- This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
Adds support for savepoints to the Connection API. This will be surfaced to users through the JDBC driver and PGAdapter.
Savepoints use the internal retry mechanism for read/write transactions to emulate actual savepoints: