Append commit instead of individual transactions to commitlog #4140
+434
−603
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This moves the following responsibilities to the datastore: - maintenance of the transaction offset - deciding how many transactions are in a commit
Allowing to restore `Committed` return
Contributor
Author
|
Nominating @gefjon and @Centril because they appeared in the reviewer suggestions. |
kim
commented
Jan 28, 2026
| /// Tests that, when a partial write occurs, we can read all flushed commits | ||
| /// up until the faulty one. | ||
| #[test] | ||
| fn reopen() { |
Contributor
Author
There was a problem hiding this comment.
I'm not sure what this was supposed to test originally, so I removed it.
kim
commented
Jan 28, 2026
| /// up until the faulty one. | ||
| #[test] | ||
| fn reopen() { | ||
| fn read_log_up_to_partial_write() { |
Contributor
Author
There was a problem hiding this comment.
This is basically the test previously named reopen.
kim
commented
Jan 28, 2026
| let writer = &mut self.head; | ||
| let committed = writer.commit(transactions)?; | ||
| if writer.len() >= self.opts.max_segment_size { | ||
| self.flush().expect("failed to flush segment"); |
Contributor
Author
There was a problem hiding this comment.
This seemed a bit surprising to me at first -- but the BufWriter has no way of knowing how many bytes did make it. So if flush fails, the buffer is basically garbage.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes the commitlog (and durability) write API, such that the caller decides how many transactions are in a single commit, and has to supply the transaction offsets.
This simplifies commitlog-side buffering logic to essentially a
BufWriter(which, of course, we must not forget to flush). This will help throughput, but offers less opportunity to retry failed writes. This is probably a good thing, as disks can fail in erratic ways, and we should rather crash and re-verify the commitlog (suffix) than continue writing.To that end, this patch liberally raises panics when there is a chance that internal state could be "poisoned" by partial writes, which may be debatable.
Motivation
The main motivation is to avoid maintaining the transaction offset in two places in such a way that they could diverge. As ordering commits is the responsibility of the datastore, we make it authoritative on this matter -- the commitlog will still check that offsets are contiguous, and refuse to commit if that's not the case.
A secondary, related motivation is the following:
A "commit" is an atomic unit of storage, meaning that a torn (partial) write of a commit will render the entire commit corrupt. There hasn't been a compelling case where we would want this, and have always configured the server to write exactly one transaction per commit.
The code to handle buffering of transactions is, however, rather complex, as it tries hard to allow the caller to retry writes at commit boundaries. An unfortunate consequence of this is that we'd flush to the OS very often, leaving throughput performance on the table.
So, if there is a compelling case for batching multiple transactions in a commit, it should be the datastore's responsibility.
API and ABI breaking changes
Breaks internal APIs only.
Expected complexity level and risk
5 - Mostly for the risk
Testing
Existing tests.