diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 1a49ee31..195b81f3 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -27,7 +27,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 runtime `AreRecordsEncrypted` gate (so default-config plaintext databases benefit without `NoEncryptMode`). Fixed-size hash-indexed SET columns are re-pointed with one lock per index (`HashIndex.RemoveBatchKeys`/`AddBatchKeys`). -- **BTree separator-delete corruption fixed (correctness)** ÔÇö `BTree.Delete` removed separator keys +- **Bulk descending PK-delete on the generic DELETE path** - `IIndex` now offers `DeleteBulk`; + `BTree.DeleteBulk` sorts each batch in **descending key order** so consecutive removals run along + the rightmost leaf path (dramatically fewer internal-separator promotions than deleting in + arbitrary per-row resolution order). `DeleteRecordsCore` collects the batch's PK keys once and + removes them through `DeleteBulk` instead of per-key `Delete`. Correctness is unchanged (identical + key set, one visit per key). Fair-PK harness (`--pk`, AppendOnly legacy): DELETE stays ~69-72K + ops/s on strictly ascending batches (within noise), with the win concentrating on unordered key + sets (hash-filtered subselects, reverse/random batches) that previously paid a separator + promotion per jump. Regression test drives the generic bulk path on a legacy-layout table and + reopens to prove tombstones + PK-index rebuild do not resurrect rows. +- **BTree separator-delete corruption fixed (correctness)** - `BTree.Delete` removed separator keys from internal nodes without repairing the child-pointer mapping, so sizable delete batches could leave whole key ranges unreachable (and scans/COUNT(*) under-counted). Internal separators are now replaced by their in-order successor from the right subtree's leftmost leaf (leaf underflow is diff --git a/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md b/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md index 2aa29651..95d3397c 100644 --- a/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md +++ b/docs/performance/EXECUTION_PLAN_UPDATE_DELETE.md @@ -11,6 +11,7 @@ | #368 | **Commit-time tombstones** (transactionele/batch deletes) | **SQL DELETE 0,82 s → 0,24 s** (~12K → ~41-58K ops/s) — de grote sprong | | #369 | C4 (batch markers + evict-dedup) + B3 (structured delete, geen dubbele parse) | veilig; neutraal binnen ruis op benchmark | | #370 | B1 (key-only decode: alleen PK + hash-indexkolommen) | veilig; neutraal binnen ruis op small-row benchmark | +| B5-bulk (open) | **Bulk aflopende PK-delete** (`DeleteRecordsCore` verzamelt PK-sleutels eenmalig; `IIndex.DeleteBulk`/`BTree.DeleteBulk` sorteert aflopend → rechter-bladpad, minder separator-promoties) | correct (identieke keyset, één bezoek per key); fair-PK legacy-DELETE ~69-72K ops/s (binnen ruis op geordende batches) — winst bij ongeordende keysets | **Root cause (niet in Grok-doc):** `ExecuteBatchSQL` draait elke batch in een storage-transactie; zonder #368 deed batch-DELETE nog steeds de #366 full-file compactie (~690 ms in `tableFlushLoop`). Daardoor waren eerdere “winst”-metingen niet-duurzaam/logisch-only. diff --git a/src/SharpCoreDB/DataStructures/BTree.cs b/src/SharpCoreDB/DataStructures/BTree.cs index 6cf4fae4..aa3980cd 100644 --- a/src/SharpCoreDB/DataStructures/BTree.cs +++ b/src/SharpCoreDB/DataStructures/BTree.cs @@ -319,6 +319,31 @@ public bool Delete(TKey key) return found; } + /// + public void DeleteBulk(IEnumerable keys) + { + var sorted = keys as TKey[] ?? keys.ToArray(); + if (sorted.Length <= 1) + { + if (sorted.Length == 1) + { + Delete(sorted[0]); + } + + return; + } + + // Descending order: consecutive deletes hit the rightmost leaf path, so the batch + // stays in (close to) a single node chain and triggers far fewer separator promotions + // / rebalances than deleting in arbitrary per-row order. Correctness is unaffected: + // every key is deleted exactly once. + Array.Sort(sorted, Comparer.Default); + for (int i = sorted.Length - 1; i >= 0; i--) + { + Delete(sorted[i]); + } + } + /// public void Clear() { diff --git a/src/SharpCoreDB/DataStructures/Table.CRUD.cs b/src/SharpCoreDB/DataStructures/Table.CRUD.cs index 7be42a7d..eaefa638 100644 --- a/src/SharpCoreDB/DataStructures/Table.CRUD.cs +++ b/src/SharpCoreDB/DataStructures/Table.CRUD.cs @@ -2633,17 +2633,24 @@ private bool HasExplicitNamedIndex(string column) Interlocked.Add(ref _pendingLogicalDeletes, recordsToDelete.Count); } - // Primary-key B-tree cleanup. + // Primary-key B-tree cleanup: bulk-delete in descending key order (one pass through the + // rightmost leaf path instead of arbitrary per-row order → fewer separator promotions). if (this.PrimaryKeyIndex >= 0) { var pkCol = this.Columns[this.PrimaryKeyIndex]; + var pkKeys = new List(recordsToDelete.Count); foreach (var (_, row) in recordsToDelete) { if (row.TryGetValue(pkCol, out var pkValue) && pkValue != null) { - this.Index.Delete(pkValue.ToString() ?? string.Empty); + pkKeys.Add(pkValue.ToString() ?? string.Empty); } } + + if (pkKeys.Count > 0) + { + this.Index.DeleteBulk(pkKeys); + } } // Key-only hash-index cleanup: extract each indexed column's key once per row and diff --git a/src/SharpCoreDB/Interfaces/IIndex.cs b/src/SharpCoreDB/Interfaces/IIndex.cs index 9b2c2918..e58a9fff 100644 --- a/src/SharpCoreDB/Interfaces/IIndex.cs +++ b/src/SharpCoreDB/Interfaces/IIndex.cs @@ -35,4 +35,17 @@ public interface IIndex /// Clears all entries from the index (used for bulk index rebuild). /// void Clear(); + + /// + /// Deletes a batch of keys from the index. The default implementation deletes per key; + /// specialized indexes may reorder the batch (e.g. descending) to reduce rebalancing cost. + /// + /// The keys to delete. + void DeleteBulk(IEnumerable keys) + { + foreach (var key in keys) + { + Delete(key); + } + } } diff --git a/tests/SharpCoreDB.Tests/FixedWidthBulkDeleteTests.cs b/tests/SharpCoreDB.Tests/FixedWidthBulkDeleteTests.cs index e3bfbc7b..ade8a7f5 100644 --- a/tests/SharpCoreDB.Tests/FixedWidthBulkDeleteTests.cs +++ b/tests/SharpCoreDB.Tests/FixedWidthBulkDeleteTests.cs @@ -277,4 +277,52 @@ public void BatchDelete_CommitTimeTombstones_SurviveReopenWithoutExplicitFlush() Assert.Equal(200L, Convert.ToInt64(scan[^1]["id"])); (db as IDisposable)?.Dispose(); } + + [Fact] + public void LegacyLayoutBatchDelete_BulkPkRemove_StaysCorrectAcrossReopen() + { + // Regression for DeleteRecordsCore's PK cleanup: with fixed-width disabled the table uses + // the legacy layout, so an ascending `pk = literal` batch cannot take the contiguous + // fast path and flows through the generic loop + Index.DeleteBulk (descending order). + // Every deleted row must vanish from PK/hash lookups and from a reopened database. + IDatabase? db = _factory.Create(_dirPath, "pw", isReadOnly: false, + config: new DatabaseConfig { NoEncryptMode = true, AutoFixedWidthRecords = false }); + try + { + db.ExecuteSQL("CREATE TABLE docs (id INTEGER PRIMARY KEY, name TEXT, score REAL)"); + db.ExecuteSQL("CREATE INDEX idx_docs_name ON docs(name)"); + InsertDocs(db, 1, 2000); + db.Flush(); + + var table = TableOf(db, "docs"); + Assert.Equal(0, table.BulkContiguousDeleteBatches); + + db.ExecuteBatchSQL(BuildDeletes(1, 1000)); + db.Flush(); + + // Legacy layout must NOT engage the fixed-width contiguous fast path. + Assert.Equal(0, table.BulkContiguousDeleteBatches); + + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE id = 500")); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'user500'")); + Assert.Single(db.ExecuteQuery("SELECT id FROM docs WHERE id = 1500")); + Assert.Single(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'user1500'")); + Assert.Equal(1000, db.ExecuteQuery("SELECT id FROM docs").Count); + } + finally { (db as IDisposable)?.Dispose(); } + + // Reopen: tombstones keep the deleted rows gone; the PK index rebuilt from the file must + // not resurrect any of them. + db = CreateDb(); + try + { + var scan = db.ExecuteQuery("SELECT id FROM docs ORDER BY id"); + Assert.Equal(1000, scan.Count); + Assert.Equal(1001L, Convert.ToInt64(scan[0]["id"])); + Assert.Equal(2000L, Convert.ToInt64(scan[^1]["id"])); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE id = 500")); + Assert.Empty(db.ExecuteQuery("SELECT id FROM docs WHERE name = 'user500'")); + } + finally { (db as IDisposable)?.Dispose(); } + } }