Indexed Logical Address Lookups - #10750
Conversation
|
@juliusvaart Please ensure this pull request is based on |
50e9022 to
8cb8566
Compare
|
Now based on |
`RealmItemMetadata.hasLocation` compared the two indexed normalized keys, but also accepted a row whose keys were empty by falling back to the raw serverUrl and fileName columns. Those columns are not indexed, and a disjunction that reaches them cannot be answered from an index, so Realm scanned the whole table instead -- once per row being ingested. The index that was added to remove that scan could never be used. The fallback existed for rows written before the normalized keys, and there are none: the addedCanonicalPathKeys migration backfilled them, and updateLocation is the only way either raw column is written since. Compare on the normalized keys alone, and repair any row that lacks them when the database is opened, so the assumption is enforced rather than trusted. Enforcing it matters because the failure is silent -- a row with empty keys does not merely take longer to find, it stops matching, so its duplicates are never evicted and it is never found by path. Ingesting 2000 rows into a 10000-row database: 1.61 s to 0.46 s. The per-directory cost also stops growing with the database, which is what made this worse the longer the client ran: across a 100x larger database it went from 1.78x to 1.12x, and at 20000 rows the ingest is 31.2 ms to 14.4 ms. Two test fixtures built rows by assigning the raw columns directly, which no production path does; they now go through updateLocation like everything else. Signed-off-by: Julius van der Vaart <julius@vanderva.art> Assisted-by: Claude Code:claude-opus-5
8cb8566 to
6c0b687
Compare
|
Artifact containing the AppImage: nextcloud-appimage-pr-10750.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
i2h3
left a comment
i2h3
left a comment
There was a problem hiding this comment.
Reviewed the branch locally. The change is correct and I would merge it — the diagnosis holds and so does the safety argument, and I checked both against primary sources rather than taking the description's word for it. One substantive suggestion that I think leaves most of the remaining win on the table, plus some documentation fallout.
What I verified
The diagnosis is right. In realm-core at the revision this package pins (b4192c46, v20.1.5), OrNode (query_engine.hpp:2113) overrides neither has_search_index() nor index_based_keys(), so the ParentNode base returns false / nullptr. An OrNode can therefore never be index-driven, exactly as the description claims. The old predicate wrapped each key in a disjunction reaching an unindexed column, so neither index was reachable.
The safety argument is right. Schema 203 (14a48a68d3) and updateLocation (9bfccd9a36) both landed in #10443 and reached master in the same merge, 77d5d00ec8. No released build ever carried schema >= 203 without updateLocation, so no shipped client can hold a row the migration missed. I also swept Sources/ for writes to the raw columns: updateLocation and RealmItemMetadata(value:) (whose first statement calls it) are the only paths, and both add() calls on already-live objects are immediately preceded by updateLocation.
Build and tests. swift build clean. Full package suite green: 354 XCTest (1 skipped) plus 59 swift-testing, 0 failures. The four new tests pass.
The one thing I would change before merging
See the inline comment on hasLocation. Short version: with the fallback gone, both conjuncts are now indexed-equality nodes, and realm-core has no selectivity estimate at plan time. ParentNode::init sets m_dD = 100.0 unconditionally (query_engine.hpp:152) and StringNodeEqualBase::init sets m_dT = 0.0 for any indexed column (query_engine.cpp:274), so both nodes score exactly 8 * 64 / 100 + 0 = 5.12. Query::find_best_node selects with std::min_element (query.cpp:1166), which returns the first minimum, so the tie is broken by source order. As written, Realm drives the query off normalizedServerUrl — the whole sibling set of the parent folder — instead of the near-unique normalizedFileName. Swapping the two operands is a one-line change.
This also inverts the rationale comment on normalizedFileName at RealmItemMetadata.swift:57-62, which says normalizedServerUrl's index is "useless for a large flat folder ... whereas the (near-unique) file name lets Realm's planner drive off this index instead". That is the intended behaviour, but it is not what the current operand order produces.
Documentation fallout
Documentation.docc/UnicodePathNormalization.md is the package's article on exactly this mechanism, and this PR makes part of it false:
- Line 66: "Both helpers retain a raw-value fallback for rows whose normalized properties are empty during migration or recovery." Not true of either helper any more.
- Line 44: names migration 203 as the only backfill mechanism. The new startup repair belongs here — the invariant the change now rests on is stated nowhere else in the docs.
Adjacent and pre-existing, not blocking
Both predate this PR. I mention them only because they sit directly on the path it touches.
FilesDatabaseManager.itemMetadata(account:locatedAtRemoteUrl:)(around line 203) takes anaccountand logs with it, but itswhereclause constrains neitheraccountnordeleted— it ishasLocationalone. So the "row occupying a logical address" lookup can return a tombstone. Every other logical-address query in that file adds!item.deleted.hasServerUrl(includingDescendants: true)is still anOrNode, andStringNode<BeginsWith>gets noIndexEvaluator(onlyStringNodeEqualBasedoes), so the descendant form still cannot use an index. The win is real but scoped to the exact-match form, which is worth knowing before someone reads the new doc comment as covering both.
Minor
- The checklist ticks the performance-test box, but no benchmark harness ships in the diff and none exists in the repo, so the numbers are not reproducible from the branch. Not asking for one — just noting the box overstates what is here.
- The green
Testscheck does not cover this Swift package;Build and test File Provider clientwas still pending when I looked.
Review produced with Claude Code (claude-opus-5), findings verified against the checked-out branch and the pinned realm-core sources.
| item.normalizedServerUrl == serverUrl.precomposedStringWithCanonicalMapping | ||
| && item.normalizedFileName == fileName.precomposedStringWithCanonicalMapping |
There was a problem hiding this comment.
Now that both conjuncts are indexed-equality nodes, the order of these two operands decides which index Realm drives the query off — and as written it picks the worse one.
realm-core has no cardinality estimate at plan time. ParentNode::init sets m_dD = 100.0 unconditionally (query_engine.hpp:152; the only other writes to m_dD are inside aggregate_local, i.e. during execution), and StringNodeEqualBase::init sets m_dT = 0.0 whenever has_search_index() (query_engine.cpp:270-283). With cost() = 8 * bitwidth_time_unit / m_dD + m_dT (query_engine.hpp:143), both nodes score exactly 8 * 64 / 100 + 0 = 5.12 — bit-identical doubles.
Query::find_best_node resolves that with std::min_element and a strict < comparator (query.cpp:1166-1174), so it returns the first minimum. Node order is source order: RealmSwift emits lhs before rhs when building the compound predicate, RLMQueryUtil walks NSAndPredicate subpredicates front to back, Query::add_node appends via add_child, and gather_children inserts the root first. So m_children[0] is the normalizedServerUrl node, it wins the tie, and do_find_all then fetches and evaluates every key in that index bucket — which for a flat folder is the entire sibling set.
Swapping the operands drives it off normalizedFileName instead, whose bucket is the rows sharing that exact normalized name table-wide. Vastly smaller than a sibling set in the /Talk-style case this targets, though not O(1):
item.normalizedFileName == fileName.precomposedStringWithCanonicalMapping
&& item.normalizedServerUrl == serverUrl.precomposedStringWithCanonicalMappingThis is not a regression — on master both conjuncts were OrNodes with no index_based_keys(), so the order was irrelevant and the PR is a strict improvement either way. It is just that the order becomes load-bearing precisely because of this change, so it is worth pinning down here with a comment noting that the tie is broken by position.
| /// A write transaction is opened only when at least one bucket has | ||
| /// more than one row, so clean databases pay no transaction cost. | ||
| /// | ||
| /// |
There was a problem hiding this comment.
The new function landed inside cleanupPreexistingLogicalDuplicates's doc comment, so the two have swapped documentation.
Everything from line 111 down to line 126 — "One-shot startup pass that heals pre-existing logical duplicates", the bucketing and winner-selection rules, the in-flight-row contract, and "A write transaction is opened only when at least one bucket has more than one row, so clean databases pay no transaction cost" — now attaches to backfillMissingNormalizedLocationKeys, which does none of those things. cleanupPreexistingLogicalDuplicates at line 155 is left with no documentation at all, and DocC will publish the deduplication description on the backfill symbol.
Moving the new /// block (128-131) and its function below cleanupPreexistingLogicalDuplicates, or just relocating the original block back onto it, fixes both.
| .where { $0.normalizedFileName == "" || $0.normalizedServerUrl == "" } | ||
| .filter { !$0.fileName.isEmpty || !$0.serverUrl.isEmpty } |
There was a problem hiding this comment.
These two predicates disagree, so the repair is not a fixed point for a row with exactly one empty raw column.
The where is a per-column emptiness test; the filter is an OR across the raw columns. A row with fileName == "" and a non-empty serverUrl satisfies both, so it is selected — but the repair writes "".precomposedStringWithCanonicalMapping, which is "", straight back into the empty side. The row still matches on the next open.
Since this is called from FilesDatabaseManager.init, that means every construction of the manager emits the logger.error at line 141 and commits a write transaction. Not a no-op at the Realm level either: Realm does not diff assignments, so identical-value writes still record modification instructions, bump the version, and fire change notifications on those rows.
Reachability is low — I could not find a production path that leaves either raw column empty (the synthesised root uses fileName: "/", the PROPFIND fixup uses nkCommonInstance.rootFileName, trash rows carry a non-empty trashUrl, and nothing in Sources/ assigns serverUrl: ""), so in practice this is log noise rather than a correctness problem. Lookups are fine regardless, because hasLocation matches "" == "" on the indexed key perfectly well.
Making the predicate a per-column drift check rather than an emptiness check makes it idempotent and covers stale keys at the same time:
.filter {
$0.normalizedFileName != $0.fileName.precomposedStringWithCanonicalMapping
|| $0.normalizedServerUrl != $0.serverUrl.precomposedStringWithCanonicalMapping
}That cannot be a Realm where clause, so it needs the indexed emptiness where kept in front of it as a cheap prefilter — or accept the scan, since cleanupPreexistingLogicalDuplicates immediately after already iterates every non-deleted row into a dictionary, which dominates it either way.
| let database = manager.ncDatabase() | ||
| try database.write { | ||
| let row = database.objects(RealmItemMetadata.self).where { $0.ocId == "renamed" }.first | ||
| row?.updateLocation(serverUrl: serverUrl, fileName: "after.txt") |
There was a problem hiding this comment.
This calls updateLocation directly, which is the thing being asserted, so the test cannot fail for the reason its name describes.
updateLocation writes both raw columns and both normalized keys in one place (RealmItemMetadata.swift:196-201), so "a rename moves the normalized keys with the row" is true here by construction. The risk the test is presumably guarding against is a rename path that forgets to go through it — and neither public entry point is exercised: FilesDatabaseManager.renameItemMetadata(ocId:newServerUrl:newFileName:) or renameDirectoryAndPropagateToChildren(ocId:newServerUrl:newFileName:), the latter being the one that rewrites descendant serverUrls in bulk.
Driving the test through renameItemMetadata would keep the same assertions and actually cover production code.
|
Note: we intend to throw out Realm as soon as possible because it is long end-of-life. I just did not get to it yet due to the overall situation. 🏠🔥 |
i2h3
changed the title
|
/backport to stable-34.0 |
2 participants
RealmItemMetadata.hasLocationcompared the two indexed normalized keys, but also accepted a row whose keys were empty by falling back to the raw serverUrl and fileName columns. Those columns are not indexed, and a disjunction that reaches them cannot be answered from an index, so Realm scanned the whole table instead -- once per row being ingested. The index that was added to remove that scan could never be used.The fallback existed for rows written before the normalized keys, and there are none: the addedCanonicalPathKeys migration backfilled them, and updateLocation is the only way either raw column is written since. Compare on the normalized keys alone, and repair any row that lacks them when the database is opened, so the assumption is enforced rather than trusted. Enforcing it matters because the failure is silent -- a row with empty keys does not merely take longer to find, it stops matching, so its duplicates are never evicted and it is never found by path.
Ingesting 2000 rows into a 10000-row database: 1.61 s to 0.46 s. The per-directory cost also stops growing with the database, which is what made this worse the longer the client ran: across a 100x larger database it went from 1.78x to 1.12x, and at 20000 rows the ingest is 31.2 ms to 14.4 ms.
Two test fixtures built rows by assigning the raw columns directly, which no production path does; they now go through updateLocation like everything else.
Assisted-by: Claude Code:claude-opus-5
Checklist
AI (if applicable)