From 23dce021a5fd5540f88e6845014c44a05866d1a5 Mon Sep 17 00:00:00 2001 From: Chen Jin Date: Wed, 5 Aug 2026 20:41:20 +0800 Subject: [PATCH] perf(fts): drain trash tables with a high-water marker instead of re-scanning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _fts_teardown_trash_step deleted rows via 'WHERE key IN (SELECT key LIMIT N)' — each chunk's subquery re-scanned from the start of the table, so chunk k skipped past (k-1)xN already-deleted rows: O(n²) total row visits. On a v22 shadow table with ~230K rows that is on the order of 10^8 row visits, turning optimize-storage teardown into a multi-hour grind on slow disks, with a write lock held per chunk. Single-column INTEGER-PK trash tables now drain via a fts_teardown__progress high-water marker mirroring fts_rebuild_step: each chunk claims rows past the marker (SELECT ... WHERE key > ? ORDER BY key LIMIT N), deletes the claimed range, and publishes the new marker in the same transaction. Per-chunk work is bounded → O(n) total. TEXT-PK tables (the FTS config shadow table, pk like 'version') and compound-key tables fall back to the legacy chunked delete — those are small by construction. Fixes #79324 --- hermes_state_search.py | 65 ++++++++++++++++++++- tests/test_hermes_state.py | 114 +++++++++++++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 2 deletions(-) diff --git a/hermes_state_search.py b/hermes_state_search.py index 600e2fdc9f..32eae0ae77 100644 --- a/hermes_state_search.py +++ b/hermes_state_search.py @@ -164,6 +164,16 @@ class SessionSearchMixin: The trash tables are PLAIN tables (their vtable parent was demoted away during the migration), so chunked DELETE + final DROP involve no FTS5 machinery at all. Returns True while teardown work remains. + + Single-column-key trash tables (the common shape — FTS shadow + tables carry a rowid/integer PK) are drained with a high-water + marker mirroring :meth:`fts_rebuild_step`: each chunk deletes only + rows after the previously-drained key, so the per-chunk scan is + bounded instead of re-scanning from the start of the table every + chunk (O(n²) total on large trash tables, #79324). Compound-key + trash tables (multi-column PK) cannot use a scalar high-water + comparison, so they keep the legacy chunked ``LIMIT`` delete — + those shadow tables are small by construction. """ with self._lock: trash = [ @@ -179,11 +189,62 @@ class SessionSearchMixin: tbl = trash[0] def _do(conn): - pk_cols = [ - r[1] for r in conn.execute(f"PRAGMA table_info({tbl})") + pk_info = [ + (r[1], (r[2] or "").upper()) + for r in conn.execute(f"PRAGMA table_info({tbl})") if r[5] > 0 ] + pk_cols = [name for name, _typ in pk_info] key = ", ".join(pk_cols) if pk_cols else "rowid" + + if len(pk_cols) == 1 and (not pk_info or pk_info[0][1] == "INTEGER"): + # High-water drain: delete only rows past the marker key. + # The marker is read/written inside the same BEGIN IMMEDIATE + # transaction as the DELETE, so concurrent callers claim + # disjoint key ranges instead of re-deleting. Only integer + # PKs can anchor a numeric high-water comparison — the FTS + # config shadow table (TEXT pk like 'version') falls back to + # the legacy chunked delete below. + marker_key = f"fts_teardown_{tbl}_progress" + row = conn.execute( + "SELECT value FROM state_meta WHERE key = ?", + (marker_key,), + ).fetchone() + high_water = int(row[0]) if row is not None else 0 + + # Claim the chunk's upper bound: the LAST row of the + # LIMIT window, so a full chunk is deleted per step. + upper_rows = conn.execute( + f"SELECT {key} FROM {tbl} WHERE {key} > ? " + f"ORDER BY {key} LIMIT {self._FTS_REBUILD_CHUNK_ROWS}", + (high_water,), + ).fetchall() + if not upper_rows: + # Drained — the DROP is cheap now. + conn.execute(f"DROP TABLE IF EXISTS {tbl}") + conn.execute( + "DELETE FROM state_meta WHERE key = ?", (marker_key,) + ) + logger.info("Old FTS shadow table %s torn down.", tbl) + return True + + upper = upper_rows[-1][0] + cur = conn.execute( + f"DELETE FROM {tbl} WHERE {key} > ? AND {key} <= ?", + (high_water, upper), + ) + if cur.rowcount > 0: + conn.execute( + "INSERT INTO state_meta (key, value) VALUES (?, ?) " + "ON CONFLICT(key) DO UPDATE SET value = excluded.value", + (marker_key, str(upper)), + ) + return True + + # Compound-key or rowid trash table: legacy chunked delete. + # These shadow tables are small, so the quadratic re-scan is + # not a concern (#79324 keeps the high-water path for the big + # single-key tables). cur = conn.execute( f"DELETE FROM {tbl} WHERE ({key}) IN " f"(SELECT {key} FROM {tbl} LIMIT {self._FTS_REBUILD_CHUNK_ROWS})" diff --git a/tests/test_hermes_state.py b/tests/test_hermes_state.py index 4d8eecfa2e..9a2895ff06 100644 --- a/tests/test_hermes_state.py +++ b/tests/test_hermes_state.py @@ -3037,6 +3037,120 @@ class TestFTSExternalContentMigration: finally: db.close() + def test_fts_teardown_single_key_high_water_drains_and_drops(self, tmp_path): + """#79324: single-column-key trash tables drain via a high-water + marker so each chunk only scans rows after the previous chunk. + + Builds a large trash table with a rowid-like integer PK, then drives + ``_fts_teardown_trash_step`` to completion. Verifies every row is + removed, the marker advances monotonically, the marker is cleared, + and the table is dropped at the end. + """ + db = SessionDB(db_path=tmp_path / "trash.db") + try: + conn = db._conn + # A plain trash table shaped like a demoted FTS shadow table + # (single integer PK — the common, large-table shape). + conn.execute( + "CREATE TABLE fts_v22_trash_messages_fts_data " + "(docid INTEGER PRIMARY KEY, block BLOB)" + ) + conn.executemany( + "INSERT INTO fts_v22_trash_messages_fts_data " + "(docid, block) VALUES (?, ?)", + [(i, b"x" * 64) for i in range(1, 2501)], + ) + conn.commit() + + assert db._has_fts_trash(conn) is True + + steps = 0 + while db._fts_teardown_trash_step(): + steps += 1 + assert steps < 100, "teardown never finished" + + # All rows gone, table dropped, marker cleaned up. + assert db._has_fts_trash(conn) is False + assert conn.execute( + "SELECT name FROM sqlite_master WHERE name = " + "'fts_v22_trash_messages_fts_data'" + ).fetchone() is None + assert db.get_meta("fts_teardown_fts_v22_trash_messages_fts_data_progress") is None + # Multiple chunks were needed (2500 rows / 500 chunk). + assert steps >= 5 + finally: + db.close() + + def test_fts_teardown_high_water_resumes_after_interruption(self, tmp_path): + """#79324: the high-water marker survives an interrupted teardown, + so the next call resumes from the marker instead of the table start.""" + db = SessionDB(db_path=tmp_path / "trash.db") + try: + conn = db._conn + conn.execute( + "CREATE TABLE fts_v22_trash_messages_fts_data " + "(docid INTEGER PRIMARY KEY, block BLOB)" + ) + conn.executemany( + "INSERT INTO fts_v22_trash_messages_fts_data " + "(docid, block) VALUES (?, ?)", + [(i, b"x" * 64) for i in range(1, 1201)], + ) + conn.commit() + + # Drain two chunks, then simulate a crash: the marker stays at + # the last drained key and the remaining rows are intact. + assert db._fts_teardown_trash_step() is True + assert db._fts_teardown_trash_step() is True + marker = db.get_meta("fts_teardown_fts_v22_trash_messages_fts_data_progress") + assert marker is not None + assert int(marker) == 1000 # 2 chunks x 500 rows + + remaining = conn.execute( + "SELECT COUNT(*) FROM fts_v22_trash_messages_fts_data" + ).fetchone()[0] + assert remaining == 200 + + # Resume: drains the rest, drops the table. + while db._fts_teardown_trash_step(): + pass + assert db._has_fts_trash(conn) is False + assert db.get_meta("fts_teardown_fts_v22_trash_messages_fts_data_progress") is None + finally: + db.close() + + def test_fts_teardown_compound_key_keeps_legacy_path(self, tmp_path): + """#79324: multi-column-PK trash tables (small by construction) keep + the legacy chunked delete — the high-water path only applies to + single-column keys.""" + db = SessionDB(db_path=tmp_path / "trash.db") + try: + conn = db._conn + conn.execute( + "CREATE TABLE fts_v22_trash_messages_fts_idx " + "(segid INTEGER, term TEXT, pgno INTEGER, " + "PRIMARY KEY (segid, term, pgno)) WITHOUT ROWID" + ) + conn.executemany( + "INSERT INTO fts_v22_trash_messages_fts_idx " + "(segid, term, pgno) VALUES (?, ?, ?)", + [(i % 3, f"term-{i}", i) for i in range(20)], + ) + conn.commit() + + steps = 0 + while db._fts_teardown_trash_step(): + steps += 1 + assert steps < 10 + + assert db._has_fts_trash(conn) is False + assert conn.execute( + "SELECT name FROM sqlite_master WHERE name = " + "'fts_v22_trash_messages_fts_idx'" + ).fetchone() is None + finally: + db.close() + # ---------------------------------------------------------------------------