Avoid snapshot in ALTER SINK if the sink has made progress #34588
Conversation
martykulma
force-pushed
the
maz-no-snapshot-alter-sink
branch
from
b37b565 to
154b7ab
Compare
patrickwwbutler
left a comment
patrickwwbutler
left a comment
There was a problem hiding this comment.
The logic seems fine to me, but I'm not well positioned to comment on the actual business repercussions of this change, so maybe worth getting another pair of eyes on it?
|
|
||
| // For `ALTER SINK`, the snapshot should only occur if the sink has not made any progress. | ||
| // This prevents unnecessary decoding in the sink. | ||
| let alter_sink_snapshot = with_snapshot && !PartialOrder::less_than(&as_of, write_frontier); |
There was a problem hiding this comment.
newbie question: would LEQ instead of strict less_than not be right here? Is it possible for the as_of and write_frontier to be exactly equal? In that case is it possible that there is actually progress being made?
There was a problem hiding this comment.
The as_of is where we are reading from, and the upper is where the next write will happen. If the write frontier hasn't progressed strictly beyond the as_of, it would mean that nothing has been written (or we are not done writing).
For a simple example, consider the case where as_of is the minimum, e.g. [0], and the upper is also the minimum [0]. In this case, I need to write the snapshot, which happens at [0], and the upper can progress (lets say it is now [1]). At this point, we know that all data associated with times less than, but not equal to, [1] are written.
|
Good find and good fix! |
4 participants
In the case of an
ALTER SINK, don't snapshot if the sink upper has progressed beyond the as_of.Motivation
fixes: https://github.com/MaterializeInc/database-issues/issues/9998
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.