[persist/source] Fixups and cleanups for the persist source write optimization #34285
+22
−17
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
bkirwi
force-pushed
the
source-fixup
branch
2 times, most recently
from
5b1eee2 to
3443a81
Compare
DAlperin
approved these changes
Nov 26, 2025
Reordering the if-else to avoid a double negative and adding comments.
We can get into a bad state if we toggle flags in the wrong way, and it's best if that bad state doesn't knock over a production environment.
One of these flags allows writing stuff that the other flag would complain about: just test the newest behaviour.
Contributor
Author
|
I backed out the commit that did more aggressive source timestamp intervals, since it seemed to trigger some unrelated issues in the parallel workload tests. (I'm not sure if they were test issues or real issues related to the sources, but in any case none of them were the assertion failure we're looking at in this PR.) I still think it would be nice to test parallel workload with a wider variety of source intervals, but I'll save that for followup work. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This PR fixes a bug in the flag check... we'd previously only enable write-time validation when that and the read-time validation flag were both enabled, but the code intended the opposite. (The effect of the old definition was that as soon as you relaxed the read-time checks, the behaviour of the writer would immediately change... not ideal!)
I think this was likely caused by the various "double negatives" in this code, so I made some small followup changes to avoid inverting booleans more than necessary and improving the docs a bit.
Motivation
Discovered while working on the rollout for https://github.com/MaterializeInc/database-issues/issues/9275 - I started seeing the behaviour change as soon as I flipped the read flag.
Tips for reviewer
Another odd thing is that we don't see any errors in the parallel workload: the code as written should error if the validations are ever disabled (allowing the new kind of writes) and then re-enabled (validating that no such writes exist).
I'm going to try and repro that case more reliably... but it's taking a minute, so I may follow up with any changes there.I've repro'd and fixed this in the last couple commits.