docs/plans/payload-index-on-disk-morph.md (3)

54-60: 💤 Low value

Clarify the cost-benefit tradeoff for Mmap → Immutable morphing.

While the document correctly states this is cheaper than a full rebuild, the "one scan of the existing mmap files to populate derived structures" (line 57) could still be expensive for large indices. The claim of "Orders of magnitude cheaper" (line 60) depends on:

• Size of the index vs size of payload storage
• Complexity of derived structure construction vs full rebuild

Consider adding:

• Rough quantitative guidance on when this optimization provides meaningful benefit (e.g., index size relative to payload storage)
• Expected wall-clock time ranges for typical index sizes
• Threshold guidance for when the scan cost might still be prohibitive

This will help implementers decide whether to add heuristics that skip the morph and fall through to rebuild for certain size ranges.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans/payload-index-on-disk-morph.md` around lines 54 - 60, Clarify the
cost-benefit tradeoff for the "Mmap → Immutable" morph by expanding the text
around the Mmap → Immutable section to include quantitative guidance: state
expected wall-clock ranges (e.g., seconds/minutes/hours) for small/medium/large
index sizes, describe cost factors (index file size vs payload storage size and
derived-structure complexity), and add a recommended threshold heuristic (e.g.,
index size or scan-time estimate beyond which to prefer full rebuild) and
mention any warm-up options like madvise(WILLNEED)/populate() and the
ImmutableX::open_mmap(inner) scan step so implementers can use these numbers to
decide whether to morph or rebuild.

---

223-223: ⚡ Quick win

Clarify crash-recovery analysis for mid-morph crashes.

Line 223 states: "morph never modifies index files, only in-memory wrappers, and save_config() is the last step. On restart, the stored config is the pre-morph config and the files match it — fully consistent."

However, the flow in lines 166-168 shows:

if self.try_morph_on_disk(field, new_on_disk)? {
    self.update_config_only(field, &payload_schema)?;
    return Ok(());
}

If try_morph_on_disk succeeds (in-memory structures are now morphed) but the system crashes before or during update_config_only → save_config(), the on-restart state is:

• Index files: unchanged (correct)
• Persisted config: old schema (pre-morph)
• In-memory structures: don't exist (restart loads from disk)

On restart, the segment loads with the old config and builds in-memory structures according to that config, which is correct. So the analysis is sound, but the explanation could be clearer:

Suggested rewording:
"Morph never modifies index files, only in-memory wrappers. save_config() is the commit point. If we crash after morph succeeds but before save_config() persists the new schema, on restart the old schema is loaded and in-memory structures are rebuilt according to the old schema from the unchanged files — fully consistent. The morph will be retried on next WAL replay of the CreateIndex operation."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans/payload-index-on-disk-morph.md` at line 223, Reword the
crash-recovery paragraph to explicitly state that try_morph_on_disk only changes
in-memory wrappers, that update_config_only (which calls save_config()) is the
durable commit point, and that if a crash occurs after try_morph_on_disk
succeeds but before update_config_only/save_config persists the new schema, on
restart the system will load the old persisted config and rebuild in-memory
structures from the unchanged index files; also add that the CreateIndex WAL
entry will cause the morph to be retried during replay. Include references to
try_morph_on_disk, update_config_only, save_config, and the CreateIndex WAL
replay to make the flow clear.

---

84-98: 💤 Low value

Clarify pseudo-code for morph_on_disk implementation.

The sketch has a few gaps that reduce clarity for design review:

1. Line 86: "construct a tiny empty variant or use take()" is underspecified. Rust enums don't have default empty variants. Options include:

   • Using Option<Self> and Option::take()
   • Defining a minimal variant like Self::Placeholder (but this pollutes the enum)
   • Using a temporary valid variant (but which one?)

2. Lines 88-94: The match arms cover Immutable ↔ Mmap transitions and the already_matching case, but the Mutable (Gridstore) case mentioned in the comment (lines 92-94) isn't shown explicitly. Should be:

   (Self::Mutable(m), _) => {
       *self = Self::Mutable(m);
       return Ok(false);
   }

3. Line 101: Failure handling says "restore the original variant" but the current value was consumed by std::mem::replace. The parenthetical suggests wrapping the operation in a Result-returning closure, but this isn't reflected in the sketch.

Consider refining the pseudo-code to show a complete, compilable pattern (even if simplified) so reviewers can validate the approach end-to-end.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docs/plans/payload-index-on-disk-morph.md` around lines 84 - 98, The
pseudo-code for NumericIndexInner::morph_on_disk is incomplete: replace the
placeholder approach with a safe take pattern (e.g., wrap self in Option<Self>
and use Option::take or std::mem::take) so you can move out the current variant
without inventing a dummy variant; add an explicit arm for the mutable case
(match (Self::Mutable(m), _) => { *self = Self::Mutable(m); return Ok(false); })
to restore and report not-applicable; and ensure error-prone conversions (like
ImmutableNumericIndex::open_mmap and imm.into_inner_mmap()) are performed inside
a Result-returning block so on Err you restore the original variant into *self
before returning the error, thereby avoiding lost state after std::mem::replace.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes