Ensure all-ephemeral transactions don't consume a tx offset #3884
+66
−15
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
Views are materialized in mutable transactions, but should not increment the transaction offset maintaintained in the committed state. This fixes storing completely empty transactions in the commitlog, and maintains that the committed state tx offset is in-sync with the commitlog's tx offset.
Centril
reviewed
Dec 16, 2025
Centril
approved these changes
Dec 16, 2025
Contributor
Centril
left a comment
Centril
left a comment
There was a problem hiding this comment.
Approved modulo the staying-lazy request, which I'd like not to change. The rest is just nice-to-haves.
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com> Signed-off-by: Kim Altintop <kim@eagain.io>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com> Signed-off-by: Kim Altintop <kim@eagain.io>
Co-authored-by: Mazdak Farrokhzad <twingoow@gmail.com> Signed-off-by: Kim Altintop <kim@eagain.io>
Shubham8287
approved these changes
Dec 16, 2025
Collaborator
|
I think this is just waiting for the csharp-testsuite fix to merge. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Merged
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 18, 2025
# Description of Changes With the addition of module-defined views, subscriptions are no longer read-only as they may invoke view materialization. The way this works is that a subscription starts off as a mutable transaction, materializes views if necessary, and then downgrades to a read-only transaction to evaluate the subscription. Before this patch, we were calling `commit_downgrade` directly on the `MutTxId` in order to downgrade the transaction. This would update the in-memory `CommittedState`, but it wouldn't make the transaction durable. This would result in us incrementing the transaction offset of the in-memory `CommittedState` without writing anything to the commitlog. This in turn would invalidate snapshots as they would be pointing further ahead into the commitlog than they should, and so when replaying from a snapshot we would potentially skip over commits that were not included in the snapshot. This patch changes those call sites to use `RelationalDB::commit_tx_downgrade` which both updates the in-memory state **and** makes the transaction durable. **NOTE:** The fact that views are materialized is purely an implementation detail at this point in time. And technically view tables are ephemeral meaning they are not persisted to the commitlog. So the real bug here was that we were updating the tx offset of the in-memory committed state at all. This is technically fixed by #3884 and so after #3884 lands this change becomes a no-op. However, we still shouldn't be calling `commit` and `commit_downgrade` directly on a `MutTxId` since in most cases it is wrong to bypass the durability layer. And without this change, the bug would still be present were view tables not ephemeral, which they may not be at some point in the future. # API and ABI breaking changes None # Expected complexity level and risk 1. The change itself is trivial, the bug is not. # Testing Adding an automated test for this is not so straightforward. First it's view related which means we don't have many options apart from a smoke test, but I don't believe the smoke tests have a mechanism for replaying the commitlog. If transaction offsets are supposed to be linear, without any gaps, then it would be useful to assert that on each append, in which case we could write a smoke test that would fail as soon as the offsets diverged.
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.
Views are materialized in mutable transactions, but should not increment the transaction offset maintained in the committed state.
This fixes storing completely empty transactions in the commitlog, and maintains that the committed state tx offset is in-sync with the commitlog's tx offset.
Expected complexity level and risk
2
Testing
Added a test.