fix(pageserver): prevent stack overflow in count_deltas#12904
fix(pageserver): prevent stack overflow in count_deltas#12904AesaKamar wants to merge 2 commits into
Conversation
AesaKamar
force-pushed
the
test-layer-map-overflow
branch
from
c3a1d22 to
0c2c57e
Compare
| let lr = lsn.start..val.get_lsn_range().start; | ||
| if !kr.is_empty() { | ||
| let base_count = Self::is_reimage_worthy(val, &key) as usize; | ||
| let new_limit = limit.map(|l| l.saturating_sub(base_count)); |
There was a problem hiding this comment.
This changed from l - base_count to l.saturating_sub(base_count). Equivalent here given base_count is 0 or 1 and Some(0) returns early, just checking the saturating was deliberate and not covering an underflow I'm not seeing.
There was a problem hiding this comment.
Hmm, There's no hidden underflow here, (I don't think at least)
Since we early-exit if limit is Some(0),l is always >= 1,, and base_count is at most <= 1. It was just a defensive change to avoid raw subtraction on usize~
| } | ||
|
|
||
| let frame_idx = frames.len(); | ||
| frames.push(Frame { max_stacked_deltas: 0 }); |
There was a problem hiding this comment.
frames only grows, it's never truncated, so it ends up holding a frame per internal node for the whole call rather than just the current path. With the bounded limit the real callers pass it stays small, but on the limit: None path it's proportional to the total number of partitions. Intentional, or worth clearing as subtrees finish?
There was a problem hiding this comment.
Thanks for reviewing. You're right, since this is DFS, we can just pop the frame when the subtree finishes. I've updated it to pop in FinishFrame so the space stays bounded by the path depth O(H) rather than the total number of partitions.
| let tenant_shard_id = TenantShardId::unsharded(TenantId::generate()); | ||
| let timeline_id = TimelineId::generate(); | ||
|
|
||
| // Insert 10,000 overlapping delta layers with increasing LSN ranges |
There was a problem hiding this comment.
Minor: the comment says 10,000 but the loop is 1..10000 so it's 9999, and they overlap on the key range, not the LSN ranges (those are disjoint). Asserting the exact 9999 is good though, it catches a semantic regression and not just the crash.
There was a problem hiding this comment.
Thanks, updated the comments to match the actual count and ranges~
2 participants
Problem
Evaluating very deep historical delta layer maps in the pageserver (such as during active GC and compaction) can cause a stack overflow in
LayerMap::count_deltasdue to recursive DFS traversal (resolving #3711).In #3711, @jcsp noted:
Summary of changes
pageserver/src/tenant/layer_map.rsinto an iterative, stack-based algorithm utilizing a heap-allocated task stack. This prevents call stack exhaustion.test_count_deltas_stack_overflowinlayer_map.rswhich verifies that evaluating a stack of 10,000 delta layers completes successfully without crashing. The test runs safely and completes in <0.3s.P.S. This is another contribution following up on my first PR! I would love any feedback on the implementation.