8000 fix(query): count only derived facts against max_results (#528) by adityamukho · Pull Request #534 · project-minigraf/minigraf · GitHub
[go: up one dir, main page]

Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- **A failed checkpoint is never retried on the same handle** (#390). A checkpoint that failed after it may have written (a failed write, a full disk, a failed `fsync`) could be retried by the next checkpoint. After a failed `fsync` the operating system may have dropped the written pages, and the checkpoint's meta page may have reached the disk although the call failed; a retry could then overwrite pages that meta references, and a crash during the retry left a file that failed to read with `STG-029`. Now every later write, checkpoint and index rebuild through that handle fails with the new `STG-046`, queries keep working, and the close-time checkpoint is skipped. Reopening the database recovers everything committed: it is all in the WAL. A `rebuild_indexes()` refused with `STG-041` writes nothing and does not count as failed. A fault-injection matrix now covers EIO, torn and short writes, a full disk and failed fsyncs at every write and sync of transacts, checkpoints, the close-time checkpoint and index rebuilds.
- **A commit made after a torn WAL entry is no longer lost on recovery** (#513). Recovery replays the WAL up to its first partial or damaged entry, but the writer appended after the end of the file. After a crash during an append, the next session's commits went in after the partial entry, so a second crash before any checkpoint lost them, although each had been fsynced and reported as committed. A failed append (short write, full disk, I/O error) did the same to the next commit through that handle. Opening a database for writing now cuts the WAL back to the end of its last valid entry, and syncs it, before anything is appended. After a failed append, every later write through that handle fails with the new `WAL-007` until the database is reopened or a `checkpoint()` succeeds. The transaction whose append failed is not applied; after a reopen it is either there in full (its entry reached the disk) or not at all. Affected every release since v0.5.0.
- **A v7 fact page with a damaged type byte fails the upgrade with `STG-014`** (#496). Opening a v7 file skipped, without an error, any page in its fact-page range whose type byte was not the fact-page type. The read-write open then upgraded the file to v8 and freed the old v7 pages, so that page's facts were lost for good; the read-only open returned the same answer with them missing. A v2.x writer fills that range with fact pages only, so any other type there is damage: open now fails with `STG-014`, read-write and read-only, and leaves the v7 file as it was. v7 fact pages have no checksum of their own, so damage elsewhere in a page can still decode to wrong facts or fail with another code; keep a copy of a v7 file until it has opened under v3.0.0.
- **Rule queries no longer fail with `INT-020` on databases with 1,000,000 or more live facts** (#528). The recursive evaluator counted the database's own facts against `max_results`, so once a database held that many live facts, every rule query failed at its first derived fact, whatever the size of its answer. `max_results` now counts only the facts the rules derive, so the `:max-results 3000000` workaround is no longer needed. Affected every release since v0.18.0. Not backported to v2.x: it is not a data-integrity fix.
- Reopening next to a WAL whose entries were all already checkpointed no longer rewinds the transaction counter, so later transactions no longer reuse `tx_count` values and `:as-of` history stays correct (#447; also fixed in v2.0.3).
- `checkpoint()` removes a WAL whose entries are all already in the file (#457). When it had no facts to flush, it returned early and left the WAL until a later write was checkpointed. That WAL is left by a crash between a checkpoint's commit and its WAL delete, or by a failed delete (`WAL-006`). Its contents were always skipped on replay, so no data was affected; the sidecar file just stayed. Not backported to v2.x: it is not a data-integrity fix.
- Closing a handle opened with `wal_checkpoint_threshold(usize::MAX)` no longer writes its pending facts to the `.graph` file (#476). The sentinel was documented to suppress the close-time checkpoint, and `Minigraf`'s close did skip it, but the storage layer still saved on drop, leaving a WAL whose entries were all checkpointed. The file now stays as the last explicit `checkpoint()` left it, and the WAL keeps every pending fact for the next open to replay, so bulk loaders and benchmarks see the pending WAL they ask for. No data is lost either way. Not backported to v2.x: it is not a data-integrity fix.
Expand Down
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -201,7 +201,7 @@ Auto-migrates v7 → v8 on open (spec §9, with a backup meta page). v1–v6 are

## Test Coverage

**1426 tests** (1416 passing, 10 ignored; unit + integration + doc).
**1427 tests** (1417 passing, 10 ignored; unit + integration + doc).
See `docs/TEST_COVERAGE.md` for the full per-file breakdown.

**Testing conventions** — see the Testing Conventions section below before writing any tests.
Expand Down
4 changes: 2 additions & 2 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ let r2 = pq.execute(&[("tx", BindValue::TxCount(2)), ("entity", BindValue::Entit

```bash
cargo run # interactive Datalog REPL
cargo test # run 1426 tests
cargo test # run 1427 tests
cargo run < demos/demo_recursive.txt # recursive rules demo
```

Expand Down Expand Up @@ -258,7 +258,7 @@ Performance envelope on the `v3` branch (format v8), measured up to 10M facts on
| Attribute scan (N/6 rows), p50 | 43 ms | 0.46 s | 4.8 s |
| Selective 2-clause join (1 row), p50 | 54 ms | 0.61 s | 6.5 s |

Point lookups and time travel on one entity stay flat at every tested size. Scans, aggregations and joins grow linearly with the facts of the attributes they touch, because a query materializes them before joining (#473). They are interactive up to about 100K facts. Not supported yet: rule queries on databases with 1M or more facts (#528), and graphs over 10M facts (not tested).
Point lookups and time travel on one entity stay flat at every tested size. Scans, aggregations and joins grow linearly with the facts of the attributes they touch, because a query materializes them before joining (#473). They are interactive up to about 100K facts. Rule queries read the whole database (#529): 2.7 s at 100K facts and 40 s at 1M. Graphs over 10M facts are not tested.

File-backed databases limit string values to 4,068 bytes (`WAL-003`). In-memory databases have no limit.

Expand Down
10 changes: 5 additions & 5 deletions docs/BENCHMARKS.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,11 +17,11 @@ This section lists the graph sizes and query shapes that are tested, with their
| Open a checkpointed file | 10M facts | 0.12 ms, 3 MiB RSS | Indexes are paged in on demand |
| Checkpoint after 1 new fact | 10M facts | 3.8 ms / 21 ms | Copy-on-write: cost follows the change |
| Attribute scan, attribute + value, grouped aggregation, joins | 1M facts | see below | Linear in the facts of the attributes they touch: about 0.1–0.5 s at 1M facts, seconds at 10M |
| Rule queries (recursion) | 100K facts | 2.7 s at 100K | Read the whole database (#529) |
| Rule queries (recursion) | 1M facts | 2.7 s at 100K, 40 s at 1M | Read the whole database (#529); not interactive past 100K |

**Not supported yet:**

- Rule queries on a database with 1,000,000 or more live facts fail with `INT-020`, whatever the size of the answer (#528). With the limit raised (`:max-results 3000000`), `recursive_ancestors` returns its 6 rows at 1M facts in 34 s.
- Interactive rule queries past 100K facts. `recursive_ancestors` takes 40 s and 3.2 GiB of RSS at 1M facts because rule evaluation reads the whole database (#529); before #528 was fixed it failed there with `INT-020`.
- Graphs over 10M facts are not tested.
- Interactive latency for scans and joins at 10M facts. They take 3–21 s and materialize every fact of the attributes they touch, 0.7–3.6 GiB of RSS.

Expand Down Expand Up @@ -53,9 +53,9 @@ This section lists the graph sizes and query shapes that are tested, with their
| `join_2` | `[:p{m} :follows ?f] [?f :name ?n]` (1 row) | 54.0 ms / 63.0 ms | 606 ms / 610 ms | 6.50 s / 7.02 s |
| `join_3_selective` | `[:p{m} :manager ?b] [?s :manager ?b] [?s :name ?n]` (10 rows) | 117 ms / 125 ms | 1.24 s / 1.28 s | 13.5 s / 13.8 s |
| `join_3_scan` | `[?e :follows ?f] [?f :group 7] [?f :name ?n]` | 150 ms / 155 ms | 1.92 s / 1.96 s | 20.9 s / 21.2 s |
| `recursive_ancestors` | `(boss :p{leaf} ?b)` over `:manager` (5–7 rows) | 2.71 s / 2.72 s | fails: `INT-020` | > 60 s (abandoned) |
| `recursive_ancestors` | `(boss :p{leaf} ?b)` over `:manager` (5–7 rows) | 2.71 s / 2.72 s | 40.1 s / 40.8 s | > 60 s (abandoned) |

Below 100 runs, p99 is the slowest run. Shapes slower than about 75 ms ran fewer than 200 times; the full JSON report has run counts.
`recursive_ancestors` at 1M was measured after the #528 fix, on the same machine and fixture. Below 100 runs, p99 is the slowest run. Shapes slower than about 75 ms ran fewer than 200 times; the full JSON report has run counts.

Peak RSS per shape, each in its own process (baseline 4 MiB):

Expand All @@ -68,7 +68,7 @@ Peak RSS per shape, each in its own process (baseline 4 MiB):
| `join_2` | 33 MiB | 307 MiB | 2,342 MiB |
| `join_3_selective` | 45 MiB | 376 MiB | 2,856 MiB |
| `join_3_scan` | 59 MiB | 456 MiB | 3,610 MiB |
| `recursive_ancestors` | 375 MiB | 2,479 MiB | 4,832 MiB (at abandonment) |
| `recursive_ancestors` | 375 MiB | 3,259 MiB | 4,832 MiB (at abandonment) |

### Fixture

Expand Down
3 changes: 2 additions & 1 deletion docs/ERROR_REFERENCE.md
Original file line number Diff line number Diff line change
Expand Up @@ -3024,7 +3024,8 @@ similar). The `{}` carries the specific underlying message.

**Cause**: The recursive rule evaluator (semi-naive evaluation) exceeded its
maximum iteration count, maximum derived facts per iteration, or maximum
query result count — guards against infinite recursion/cycles or
number of facts derived in total (`max_results`; the database's own facts do
not count) — guards against infinite recursion/cycles or
runaway-generating rules.

**Resolution**:
Expand Down
4 changes: 2 additions & 2 deletions docs/TEST_COVERAGE.md

Large diffs are not rendered by default.

8 changes: 5 additions & 3 deletions src/db.rs
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,8 @@ pub struct OpenOptions {
/// Maximum facts that can be derived per recursive rule iteration.
/// Defaults to 1_000_000. Use to prevent runaway recursive rules.
pub max_derived_facts: usize,
/// Maximum total query results. Defaults to 1_000_000.
/// Maximum facts a rule query may derive. Defaults to 1_000_000.
/// The database's own facts do not count against it.
pub max_results: usize,
/// Open even when the filesystem cannot lock files at all.
///
Expand Down Expand Up @@ -228,9 +229,10 @@ impl OpenOptions {
self
}

/// Set the maximum total query results.
/// Set the maximum number of facts a rule query may derive.
///
/// Defaults to 1_000_000. Use lower values to limit result set size.
/// Defaults to 1_000_000. The database's own facts do not count against it,
/// and queries without rules are not limited by it.
pub fn max_results(mut self, n: usize) -> Self {
self.max_results = n;
self
Expand Down
10 changes: 6 additions & 4 deletions src/query/datalog/evaluator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ pub struct RecursiveEvaluator {
max_iterations: usize,
/// Maximum facts that can be derived per iteration
max_derived_facts: usize,
/// Maximum total query results
/// Maximum facts the rules may derive in one evaluation; base facts do not count
max_results: usize,
}

Expand All @@ -81,7 +81,7 @@ impl RecursiveEvaluator {
/// * `functions` - Function registry for UDF predicates/aggregates
/// * `max_iterations` - Safety limit (e.g., 1000)
/// * `max_derived_facts` - Maximum facts per iteration
/// * `max_results` - Maximum total results
/// * `max_results` - Maximum facts the rules may derive (base facts do not count)
pub fn new(
storage: FactStorage,
rules: Arc<RwLock<RuleRegistry>>,
Expand Down Expand Up @@ -133,6 +133,8 @@ impl RecursiveEvaluator {
(f.entity, f.attribute.clone(), encoded)
})
.collect();
// `max_results` limits the facts the rules derive, not the database (#528).
let base_count = seen_facts.len();

let mut iteration = 0;

Expand Down Expand Up @@ -170,8 +172,8 @@ impl RecursiveEvaluator {
let encoded = encode_value(&fact.value);
let key = (fact.entity, fact.attribute.clone(), encoded);
if !seen_facts.contains(&key) {
// Check total result limit
if seen_facts.len() >= self.max_results {
// Check total derived-fact limit
if seen_facts.len() - base_count >= self.max_results {
return Err(err_coded!(
ErrorCode::Int020,
format!("Max query results ({}) exceeded.", self.max_results)
Expand Down
35 changes: 35 additions & 0 deletions tests/recursive_rules_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -468,3 +468,38 @@ fn test_convergence_simple_graph() {
_ => panic!("Expected QueryResults"),
}
}

/// #528: `max_results` limits what the rules derive, not the size of the database.
/// A rule query must answer on a database with more live facts than `max_results`.
#[test]
fn test_max_results_counts_derived_facts_not_base_facts() {
let db = minigraf::OpenOptions::new()
.max_results(10)
.open_memory()
.unwrap();
// A chain a -> b -> c -> d, plus 50 unrelated facts: 53 live facts in all.
exec(
&db,
"(transact [[:a :manager :b] [:b :manager :c] [:c :manager :d]])",
);
let filler: String = (0..50).map(|i| format!("[:p{i} :age {i}] ")).collect();
exec(&db, &format!("(transact [{filler}])"));
exec(&db, "(rule [(boss ?x ?y) [?x :manager ?y]])");
exec(&db, "(rule [(boss ?x ?y) [?x :manager ?m] (boss ?m ?y)])");

// Bound first argument (magic-sets path): b, c, d.
match exec(&db, "(query [:find ?y :where (boss :a ?y)])") {
QueryResult::QueryResults { results, .. } => assert_eq!(results.len(), 3),
_ => panic!("Expected QueryResults"),
}
// All free: 6 pairs.
match exec(&db, "(query [:find ?x ?y :where (boss ?x ?y)])") {
QueryResult::QueryResults { results, .. } => assert_eq!(results.len(), 6),
_ => panic!("Expected QueryResults"),
}
// The limit still stops a rule that derives more than it allows.
let err = db
.execute("(query [:find ?x ?y :where (boss ?x ?y) :max-results 5])")
.unwrap_err();
assert_eq!(err.code(), "INT-020");
}
Loading
0