Skip to content

Commit 8553bf0

Browse files
CopilotXuPeng-SH
andauthored
fix: multi-table DELETE+LIMIT syntax error and inaccurate index comment in cleanup_orphan_stats (#115)
Two issues in the batched `cleanup_orphan_stats` rewrite and one misleading comment introduced by the previous perf commit. ## Bug fix: multi-table DELETE with LIMIT is invalid MySQL/MatrixOne syntax `DELETE alias FROM t1 LEFT JOIN t2 … LIMIT n` is rejected at runtime — MySQL forbids `ORDER BY`/`LIMIT` on multi-table deletes. Every other batched `DELETE` in the file is single-table for exactly this reason. Replaced with the standard two-step pattern used elsewhere: ```sql -- Step 1: SELECT up to 1000 orphan IDs (LIMIT valid on SELECT) SELECT s.memory_id FROM mem_memories_stats s LEFT JOIN mem_memories m ON s.memory_id = m.memory_id WHERE m.memory_id IS NULL LIMIT 1000; -- Step 2: single-table DELETE by PK — no LIMIT restriction DELETE FROM mem_memories_stats WHERE memory_id IN (?, …); ``` `mem_memories_stats` has `memory_id` as its `PRIMARY KEY` (one row per memory), so the SELECT never returns duplicates and the loop terminates correctly. ## Comment fix: `idx_memories_user_observed` scope The original comment claimed the index also accelerates `archive_stale_working`. It doesn't — that function uses `TIMESTAMPDIFF(HOUR, observed_at, NOW()) > ?`, which wraps `observed_at` in a function and prevents a B-tree range scan. The index only helps `health_capacity()`'s direct range predicate `observed_at >= NOW() - INTERVAL 30 DAY`. Comment corrected accordingly. <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/matrixorigin/Memoria/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: XuPeng-SH <39627130+XuPeng-SH@users.noreply.github.com>
1 parent fd92c54 commit 8553bf0

1 file changed

Lines changed: 92 additions & 10 deletions

File tree

  • memoria/crates/memoria-storage/src

memoria/crates/memoria-storage/src/store.rs

Lines changed: 92 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -784,6 +784,51 @@ impl SqlMemoryStore {
784784
.await;
785785
}
786786

787+
// Migration: add composite index (user_id, memory_id) on mem_retrieval_feedback.
788+
// The existing idx_feedback_user(user_id, created_at) does not cover the JOIN on
789+
// memory_id used by get_feedback_by_tier(), causing a full-table scan on feedback.
790+
let has_feedback_memory_user_idx: bool = sqlx::query_scalar(
791+
"SELECT COUNT(*) > 0 FROM information_schema.statistics \
792+
WHERE table_schema = DATABASE() \
793+
AND table_name = 'mem_retrieval_feedback' \
794+
AND index_name = 'idx_feedback_memory_user'",
795+
)
796+
.fetch_one(&self.pool)
797+
.await
798+
.unwrap_or(false);
799+
if !has_feedback_memory_user_idx {
800+
let _ = sqlx::query(
801+
"ALTER TABLE mem_retrieval_feedback \
802+
ADD INDEX idx_feedback_memory_user (user_id, memory_id)",
803+
)
804+
.execute(&self.pool)
805+
.await;
806+
}
807+
808+
// Migration: add (user_id, observed_at) index on mem_memories.
809+
// Speeds up the monthly-growth-rate count in health_capacity() which uses
810+
// `observed_at >= NOW() - INTERVAL 30 DAY` (direct range comparison).
811+
// Note: TIMESTAMPDIFF-wrapped predicates (e.g. archive_stale_working) cannot
812+
// use a B-tree range scan regardless of the index; they are covered by the
813+
// existing idx_user_active (user_id, is_active, memory_type) instead.
814+
let has_memories_user_observed_idx: bool = sqlx::query_scalar(
815+
"SELECT COUNT(*) > 0 FROM information_schema.statistics \
816+
WHERE table_schema = DATABASE() \
817+
AND table_name = 'mem_memories' \
818+
AND index_name = 'idx_memories_user_observed'",
819+
)
820+
.fetch_one(&self.pool)
821+
.await
822+
.unwrap_or(false);
823+
if !has_memories_user_observed_idx {
824+
let _ = sqlx::query(
825+
"ALTER TABLE mem_memories \
826+
ADD INDEX idx_memories_user_observed (user_id, observed_at)",
827+
)
828+
.execute(&self.pool)
829+
.await;
830+
}
831+
787832
Ok(())
788833
}
789834

@@ -1376,12 +1421,16 @@ impl SqlMemoryStore {
13761421
window_days: i64,
13771422
max_pairs: usize,
13781423
) -> Result<i64, MemoriaError> {
1424+
// Cap the fetch at 5,000 rows to bound memory usage: each embedding can be
1425+
// several KB, so loading unbounded rows risks exhausting heap for active users.
1426+
// The max_pairs limit already caps pair-comparison work in the loop below.
13791427
let rows: Vec<(String, String, chrono::NaiveDateTime, String)> = sqlx::query_as(
13801428
"SELECT memory_id, memory_type, observed_at, embedding \
13811429
FROM mem_memories \
13821430
WHERE user_id = ? AND is_active = 1 AND embedding IS NOT NULL \
13831431
AND TIMESTAMPDIFF(DAY, observed_at, NOW()) <= ? \
1384-
ORDER BY memory_type, observed_at DESC",
1432+
ORDER BY memory_type, observed_at DESC \
1433+
LIMIT 5000",
13851434
)
13861435
.bind(user_id)
13871436
.bind(window_days)
@@ -1681,16 +1730,49 @@ impl SqlMemoryStore {
16811730
}
16821731

16831732
/// Clean up orphaned stats records (stats without corresponding memory).
1733+
/// Runs in batches of 1,000 to limit lock pressure.
1734+
///
1735+
/// Multi-table DELETE with LIMIT is not valid MySQL/MatrixOne syntax, so we
1736+
/// first SELECT the orphan IDs and then DELETE them by primary key.
16841737
pub async fn cleanup_orphan_stats(&self) -> Result<i64, MemoriaError> {
1685-
let result = sqlx::query(
1686-
"DELETE s FROM mem_memories_stats s \
1687-
LEFT JOIN mem_memories m ON s.memory_id = m.memory_id \
1688-
WHERE m.memory_id IS NULL",
1689-
)
1690-
.execute(&self.pool)
1691-
.await
1692-
.map_err(db_err)?;
1693-
Ok(result.rows_affected() as i64)
1738+
const BATCH: i64 = 1000;
1739+
let mut total = 0i64;
1740+
loop {
1741+
// Step 1: collect up to BATCH orphan IDs.
1742+
let ids: Vec<(String,)> = sqlx::query_as(
1743+
"SELECT s.memory_id \
1744+
FROM mem_memories_stats s \
1745+
LEFT JOIN mem_memories m ON s.memory_id = m.memory_id \
1746+
WHERE m.memory_id IS NULL \
1747+
LIMIT 1000",
1748+
)
1749+
.fetch_all(&self.pool)
1750+
.await
1751+
.map_err(db_err)?;
1752+
1753+
if ids.is_empty() {
1754+
break;
1755+
}
1756+
1757+
// Step 2: delete by primary key (single-table, so LIMIT is allowed, though
1758+
// not needed here since the IN-list is already capped at BATCH).
1759+
let placeholders: Vec<&str> = ids.iter().map(|_| "?").collect();
1760+
let sql = format!(
1761+
"DELETE FROM mem_memories_stats WHERE memory_id IN ({})",
1762+
placeholders.join(", ")
1763+
);
1764+
let mut q = sqlx::query(&sql);
1765+
for (id,) in &ids {
1766+
q = q.bind(id);
1767+
}
1768+
let n = q.execute(&self.pool).await.map_err(db_err)?.rows_affected() as i64;
1769+
total += n;
1770+
1771+
if (ids.len() as i64) < BATCH {
1772+
break;
1773+
}
1774+
}
1775+
Ok(total)
16941776
}
16951777

16961778
/// Delete old audit-log rows older than `retain_days` days, in batches to avoid lock pressure.

0 commit comments

Comments
 (0)