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.
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_downgradedirectly on theMutTxIdin order to downgrade the transaction. This would update the in-memoryCommittedState, but it wouldn't make the transaction durable.This would result in us incrementing the transaction offset of the in-memory
CommittedStatewithout 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_downgradewhich 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
commitandcommit_downgradedirectly on aMutTxIdsince 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
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.