# Архитектурное ревью armdb (Design & Code)

Я внимательно изучил дизайн-документ (`design.md`) и исходный код (в частности модули `shard.rs`, `compaction.rs`, `const_tree.rs`, `var_tree.rs`, `skiplist/mod.rs` и `skiplist/node.rs`). 

**Общее впечатление:** Дизайн отличный и очень хорошо подходит под заявленные цели (оптимизация под NVMe, предсказуемая задержка). Разделение на `ConstTree` (zero-I/O чтение) и `VarTree` (S3-FIFO Block Cache) — блестящее решение. Concurrent SkipList на основе `seize` также реализован чисто. Однако в реализации есть **одна критическая ошибка (утечка)** и **одна колоссальная упущенная оптимизация производительности**, которая сильно портит идею шардирования.

Ниже подробный разбор.

---

### 1. Критическая ошибка: «Утечка» `dead_bytes` при компакции
**Где:** `armdb/src/compaction.rs`

Когда фоновый компактор переносит живые записи в новый файл (`tmp_file`), он собирает их в `batch`. Затем он блокирует шард и применяет изменения к индексу:
```rust
let _inner = shard.lock();
for entry in batch.drain(..) {
    if index.update_if_match(&entry.key, entry.old_loc, entry.new_loc) {
        compacted_entries += 1;
    } else {
        // [!] ЗДЕСЬ ОШИБКА
    }
}
```
Если `update_if_match` возвращает `false`, это означает, что пока компактор писал данные в `tmp_file`, какой-то параллельный `put` **уже обновил этот ключ** в `active_file`.
* Что сделал параллельный `put`: Он записал новые данные, обновил индекс и добавил размер старой записи (из старого файла) в `dead_bytes`.
* Что сделал компактор: Он записал уже **устаревшую (мертвую)** запись в `entry.new_loc` (то есть в новый сжатый файл).

**Проблема:** Ветка `else` пуста. Новая запись в сжатом файле является "мусором" с самого момента создания, но вы **не добавляете ее размер в `dead_bytes` для нового файла**. Из-за этого у сжатых файлов появляется "скрытый мусор", о котором компактор не знает. Со временем это может привести к утечке дискового пространства, так как `garbage_ratio` файла никогда не достигнет порога (`threshold`), даже если файл целиком состоит из мертвых записей.

**Как исправить:**
```rust
if index.update_if_match(&entry.key, entry.old_loc, entry.new_loc) {
    compacted_entries += 1;
} else {
    // Эта запись в новом файле уже мертва!
    let dead_size = crate::entry::entry_size(K, entry.new_loc.len);
    _inner.add_dead_bytes(entry.new_loc.file_id, dead_size);
}
```

---

### 2. Критично для производительности: Глобальный лок SkipList при Update
**Где:** `armdb/src/const_tree.rs` и `armdb/src/var_tree.rs` (методы `put_locked`)

В `design.md` вы пишете, что `SkipList` сериализует вставки через `write_lock: Mutex<()>`, и что это "бутылочное горлышко" для параллельных записей из разных шардов. 

Но в коде `put_locked` вы делаете так:
```rust
let node_ptr = ConstNode::alloc(*key, *value, disk_loc, height); // Выделяем память
match self.index.insert(node_ptr, guard) { // БЕРЕМ ГЛОБАЛЬНЫЙ write_lock!
    InsertResult::Exists(existing) => {
        // ... делаем swap_data ...
        drop(Box::from_raw(node_ptr)); // Выкидываем аллокацию
    }
}
```
Метод `put_locked` вызывается **под локом конкретного шарда** (`let mut inner = shard.lock();`). Это означает, что никакой другой поток в принципе не может писать в этот же ключ параллельно! 

**Вывод:** Вам **не нужно** брать глобальный `write_lock` SkipList-а, если ключ уже существует. Вы можете сделать lock-free `get()` (который ничего не блокирует), и если ключ есть, просто обновить атомарный указатель:

**Как исправить (ускорит updates в десятки раз):**
```rust
fn put_locked(...) -> DbResult<()> {
    let (disk_loc, _gsn) = inner.append_entry(shard_id as u8, key, value, false)?;

    // 1. Быстрая lock-free проверка: если ключ есть, просто атомарно меняем данные!
    if let Some(existing) = self.index.get(key, guard) {
        let new_disk = Box::into_raw(Box::new(disk_loc));
        let old_disk_ptr = existing.swap_disk(new_disk);
        // ... (обновляем dead_bytes и отправляем в retire) ...
        return Ok(());
    }

    // 2. И только если ключа НЕТ, берем глобальный write_lock через insert
    let node_ptr = VarNode::alloc(*key, disk_loc, random_height());
    self.index.insert(node_ptr, guard);
    Ok(())
}
```
Для рабочих нагрузок, где преобладают обновления существующих ключей, это полностью уберет конкуренцию между шардами за `SkipList::write_lock` и избавит от лишней аллокации узла.

---

### 3. Неэффективность: Слепая запись Tombstone в `VarTree::delete`
**Где:** `armdb/src/var_tree.rs`

В `ConstTree::delete_locked` вы разумно проверяете наличие ключа в памяти перед тем, как писать на диск (если ключа нет, возвращаете `Ok(None)`). 
Но в `VarTree::delete_locked` вы сначала **всегда пишете tombstone на диск**, и только потом вызываете `index.remove(key)`:
```rust
fn delete_locked(...) -> DbResult<bool> {
    inner.append_entry(shard_id as u8, key, &[], true)?; // Пишем на диск ВСЕГДА
    let removed = self.index.remove(key, guard);
    // ...
}
```
Если ключа не существовало, вы впустую тратите дисковое пространство, инкрементируете `GLOBAL_GSN` и нагружаете io_uring записью фантомного tombstone. 

**Как исправить:** Добавьте быструю lock-free проверку, как в `ConstTree`:
```rust
if self.index.get(key, guard).is_none() {
    return Ok(false);
}
// только потом append_entry
```

---

### 4. Не критичные архитектурные наблюдения

1. **CAS (Compare-and-Swap) блокирует шарды на I/O.** В `VarTree::cas` вы берете лок шарда, а затем вызываете `get_under_lock`, который может сделать `pread` мимо кэша. На время этого дискового чтения **все записи** в этот шард (даже для других ключей) будут заблокированы. В рамках вашей архитектуры (strict sharding) это ожидаемо и нормально, но стоит помнить об этом при проектировании систем, полагающихся на `cas`.
2. **Память HashMap в `migrate()`:** Для `ConstMap` и `VarMap` миграция требует сбора всех ключей шарда в память перед итерацией (упомянуто в доке как $O(K \times N/shards)$). Если база разрастется до сотен миллионов ключей, это может вызвать сильный спайк потребления памяти при запуске. 
3. **Безопасность `is_marked()` при обходе SkipList:** Реализация RCU в `node.rs` и помощь читателей при `unlink` (в `SkipList::get` и `insert`) написана предельно грамотно и без гонок. `write_lock` на удалении элегантно страхует логику от конфликтов двух писателей.

### Итог
Архитектура надежная и отлично продумана. Обязательно добавьте обработку ветки `else` в компакторе (чтобы предотвратить скрытую утечку диска) и добавьте Bypass через `get()` в методах `put_locked` — это бесплатно даст вашей БД колоссальный буст производительности при конкурентных `update` операциях.