feat(engine): let the schema dictionary grow past a single page - #5560
Conversation
The dictionary held every type name, property name and enumerated string value in page 0 alone, so a database was capped at whatever fitted there: 327,668 bytes, measured at 48,396 short names. Past that, CREATE PROPERTY and inserting a document with a new field name failed permanently, with no way to grow. Names now roll over to a new page. Every page, page 0 included, carries the same 4 byte legacy counter, so a dictionary written before this change is exactly a dictionary of one page and loads with no special case: no migration, no rewrite, no format flag needed to read an existing database. An id is the ordinal of the name in page order and that id is written inside records on disk, so the layout is strictly append-only across pages. Names go only to the LAST page; a page left behind is never revisited even when it still has room. Filling a gap would renumber every name after it and silently repoint every record that referenced them, which is the one way this change could corrupt data rather than throw. The wasted tail is under 1% at identifier lengths, and a name is never split across pages, so the only limit left is a single name larger than one page, which is now refused by name. addItemToPage reads the tail page before deciding where to write: asking for it as modifiable and then finding it full would bump its version, rewrite it at commit for nothing, and make it false-conflict with concurrent transactions. reload() walks the committed page count rather than getTotalPages(), because TransactionContext.rollback() calls it while the transaction's own page counter still counts pages that are being thrown away. The floor of 1 keeps the previous behaviour of always reading page 0 of a non-empty file. updateName() re-lays the whole dictionary out from page 0 in the same order, so no id moves, and empties the pages the new content no longer reaches: reload() walks every committed page, and a stale tail page would re-add its old names. The page size drops from 327,680 to 65,536 for new dictionaries. That size existed only to make the one available page hold as many names as possible; now that pages roll over, a new name dirties and eventually flushes one page, so a smaller page is five times less write amplification per name. Existing databases keep the page size they were created with, read back from the file name. Format version goes to 1, with a guard refusing a dictionary written by a newer ArcadeDB instead of misreading it. Note the reverse: once a database grows past page 0 it can no longer be opened by an older ArcadeDB, which reads page 0 only and reports "Dictionary item with id N is not valid". Such a database could not have existed before this change, since the write would have been refused.
|
Tick the box to add this pull request to the merge queue (same as
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 10 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Review:
|
Drop getAvailableSpaceInLastPage(). It had no caller left once the test that used it was rewritten, and dead public API on an engine class is a liability; the one test that needs the tail page's free space lives in the same package and reads the page directly. Validate newName up front in updateName(), before anything is mutated. The rewrite edits the in-RAM view first, so a name too big for a page used to leave the dictionary renamed but unwritten until the caller's rollback happened to repair it. Every other name is already stored on a page of this size, so newName is the only one that can fail. The per-name check inside the rewrite loop stays: it costs two arithmetic operations and is the only thing standing between a corrupt page and a raw "cannot write outside the page space". Chain the cause into the SchemaException updateName() throws on IOException, as addItemToPage() already does. Two tests for paths that were not covered: renaming to a name that no longer fits the existing pages, which is the branch of resetPageForRewrite that adds one, and opening a database whose dictionary claims a newer format version, exercised through the real load path by renaming the file rather than by calling the constructor directly. Not changed: the pages updateName() empties when the content shrinks stay in the file. Truncating would be the complete fix, but page counts are tracked by PaginatedComponent.pageCount, TransactionContext.newPageCounters and TransactionManager.applyChanges on the follower, so shrinking a live file id risks a follower replaying a WAL entry for a page number that no longer exists. That is real risk against a method with no production callers, to reclaim pages that cost one zero-entry read each.
|
Thanks, all six points reviewed. Four applied, one applied slightly differently from the suggestion, one declined with reasoning. 1. 2. 3. 4. Orphaned pages after a shrink — declining, and I want to be explicit about why rather than just calling it rare. Truncating the file is the obvious "complete" fix, but page counts are tracked in three places that would disagree with a shrunk file: 5. Grow branch of 6. No test for the version guard — added, but not as a constructor unit test. Calling the constructor directly would prove the Local: 18 dictionary tests, then 623 tests across |
|
Review: feat(engine): let the schema dictionary grow past a single page Reviewed statically against the surrounding engine ( Overall this is a strong, carefully reasoned change. The invariant is stated crisply ("an id is the ordinal of the name in page order"), the on-disk backward compatibility is genuinely zero-cost (page 0 keeps the exact legacy shape, version 0 loads unchanged), and the comments explain the why behind each subtle decision rather than restating the code. The tests are unusually thorough for this kind of format change. Things I verified and agree with
Minor points (non-blocking)
Style, TDD, Nice work. |
…saction's Writing the test the review asked for, for a local transaction that grows the dictionary onto a new page and then rolls back, showed the reasoning it was meant to pin did not actually hold. reload() read its pages through TransactionContext.getPage(), which resolves modifiedPages FIRST. On the rollback path that means it rebuilt the in-RAM dictionary from exactly the content being discarded: after a rolled back updateName, the new name still resolved to the renamed id until something else reloaded. Reading through the page manager instead makes both halves of the method agree on durable state, matching the pageCount choice already made for the page count. Every caller wants committed content: load, rollback, and the two post-commit replication paths. The defect predates multi-page support, since updateName always modified page 0 inside the caller's transaction. Confirmed the test pins the fix by reverting the read and watching it fail. Only updateName can reach it: it runs inside the caller's transaction and both modifies existing pages and adds new ones, so a dictionary page lands in modifiedPages, which is what arms the reload on rollback. An append through getIdByName only ever adds a page and never modifies one, so it never arms it. Also state in the class javadoc which strings can enter the dictionary. Only identifiers are ever added, the create=true callers: type names and property names. A string value is only ever looked up with create=false and stored as a reference when it already matches an entry, so user data never enters and the per-name size limit only ever applies to identifiers. The old wording, "enumerated string value", read as though user data could land here and made the smaller page size look like a limit on values.
|
Round 2. Point 4 turned out to be the valuable one: writing the test found a real defect, and it is not one this PR introduced. 4. No test for a local transaction crossing a page boundary then rolling back — agreed, and worth spelling out why the path is narrower than it looks. And it failed. Fixed by reading through the page manager, so both halves agree. Every caller wants committed content: load, rollback, and the two post-commit replication paths. This defect predates multi-page support — 1. "enumerated string value" reads like user data — agreed, and I fixed it in the class javadoc rather than only in the PR body. The PR body is read once; the javadoc is where the question comes back. It now says explicitly that only identifiers are ever added (the 2. New single-page v1 files are not refused by old versions — correct, and intended. A fresh database stays readable by an older ArcadeDB for as long as it fits page 0, and only becomes unreadable once it actually uses the new capability. Downgradability is lost exactly when the database starts depending on the feature, not a moment earlier. No code change; flagging it here so the note is not misread. 3. Redundant Local: 624 tests across |
|
Review: multi-page schema dictionary (PR #5560) Reviewed the full diff and traced the surrounding code paths ( What I verified holds
Points worth a look (none blocking)
Test coverage Strong. Rollover, reopen persistence, legacy page-0 layout, oversized-name rejection leaving the dictionary usable, the serializer round-trip across pages, Overall: I did not find a correctness or data-integrity defect. The design is sound, the append-only guarantee is preserved end-to-end, and the durability/rollback edge cases the change touches are handled and tested. The notes above are documentation/operational polish, not blockers. Note: I was unable to run the build in this sandbox; this review is from code analysis. The author reports a clean local run (920 engine test classes, 0 failures). |
…ade order Two behaviour changes were only visible by reading the diff. The smaller page size caps a single identifier at pageSize - 12 bytes, so ~65Kb on a new dictionary against ~327Kb on one created before. Only new databases see it, and only identifiers are subject to it since string values are never inserted, but the class javadoc now says so instead of leaving it to be derived. More important operationally: dictionary pages replicate as raw pages, so a leader that has rolled the dictionary over ships page 1 and beyond to its followers. A follower still running a build without multi-page support writes the page but reloads only page 0, so every record referencing a name past the first page fails with "Dictionary item with id N is not valid". Followers have to be upgraded before, or together with, the leader. Recorded in the class javadoc and in docs/5560-dictionary-multipage.md, which also collects the design, the page-size and version decisions, and the single-node and downgrade notes. Also note that createIfNotExists=true in the reload() page read is load bearing rather than a default: paired with the Math.max(1, pageCount) floor it materialises page 0 of a file shorter than one page, which is what the single-page reader did. Not tagging DictionaryLimitsTest.fillingWellPastOnePageKeepsWorking as slow: it measures 0.3s, and every DictionaryMultiPageTest case is at or under 0.088s, so tagging would drop real coverage from any run that skips slow tests for no gain.
|
Round 3. All four applied, one of them by measuring rather than doing what was suggested. 1. Identifier cap shrinks 5x on new databases — agreed, and it belongs in the class javadoc rather than only the PR text, which is read once. It now states the cap as 2. HA rolling-upgrade ordering — the most valuable point of this round, and I agree it would not be inferred from the file-format note. Recorded in two places: the class javadoc, next to the code that causes it, and I considered whether this could be prevented rather than documented — a leader refusing to roll over while any follower is on an older build. That needs cluster-wide version negotiation and would couple the engine to 3. 4. Local: 19 dictionary tests, 0 failures. Previous round's wider run was 624 tests across |
|
Review: feat(engine): let the schema dictionary grow past a single page Thorough, well-reasoned change. The PR description and the class Javadoc do a lot of the reviewer work, and the invariants they state hold up against the code. I traced the append path, the What is done well
Minor points (non-blocking)
Verification Overall: a careful, well-documented change with matching tests. The minor points above are polish, not blockers. |
…t it proves updateName found each occurrence with indexOf() and rewrote it until none were left, re-scanning the whole list per occurrence. Scan by index instead: one pass instead of one per occurrence, and the loop can no longer fail to terminate when oldName equals newName, which previously depended on the two names differing. The early return for that case stays, but it is now a fast path rather than the only thing standing between the loop and spinning forever. fillingWellPastOnePageKeepsWorking inserted 20,000 names, each committing its own nested transaction. The point of the test is that the dictionary keeps working well past one page, and 3,000 padded names cross four of them, so the extra 17,000 WAL commits proved nothing. 0.3s to 0.068s, which keeps it well under a second even on a slow CI runner and leaves it out of the slow tag, where it would have cost real coverage on every run that skips those.
|
Round 4. Two applied, one stale, one agreed as non-actionable. 1. 2. 3. Test timing on a slow runner — fair push-back on my local 0.3s measurement; my machine is not a 2-vCPU runner. But between the two options I think there is a third that beats both. The test exists to show the dictionary keeps working well past one page, and 3,000 padded names cross four pages just as convincingly as 20,000 do — the other 17,000 bought nothing but WAL commits. Resized: 0.3s to 0.068s, a 4.4x cut with the coverage intact, and I strengthened the assertion from 4. Concurrency corner in On verification: agreed that green CI on |
Review: multi-page schema dictionaryStrong, carefully reasoned change. The append-only invariant ("an id is the ordinal of the name in page order"), the backward-compatible page-0 layout, the A few observations: 1.
|
…nything Moving the newName size check ahead of the mutation left two validations behind it. The type-name check is the one that matters: by the time it fires, the list has already been renamed and the map has already lost oldName. It throws IllegalArgumentException, which the local catch does not handle since that only catches IOException, and no dictionary page has been written yet, so TransactionContext.rollback() does not arm its repairing reload either. The in-RAM view was left renamed in the list and missing from the map until some unrelated reload came along. All four preconditions now run first, then a read-only scan, then the mutation. The type-name check moved above the scan rather than just above the mutation: it depends only on oldName, so a rename refused for that reason no longer walks the dictionary at all. After the scan the only way out is an IOException, which is exactly what the catch repairs. Tests: renaming a name that is also a type name now asserts both directions of the mapping still agree afterwards, with no reload to repair them. Verified it fails before the fix. Also a contention test, which nothing covered: four threads appending distinct names across a rollover, asserting no two ids collide, every id round-trips, and the ids handed out under contention still match the order the names landed in on the pages after a reload. That last assertion is the one that would catch two threads computing the tail page at once.
|
Round 5. Point 1 was right, and I took it further than proposed. Point 3's second half was also worth acting on. 1. Two validations still ran after the in-RAM mutation — confirmed exactly as you traced it, including the part that makes it bite: You proposed computing Regression test added as suggested, and I verified it fails against the old ordering: 3, second half: no contention test — agreed, and this was a real gap, since the class's whole safety argument is "appends serialise on the monitor while 2. Release notes / upgrade guide — the mechanism and the "upgrade followers before, or together with, the leader" instruction are in 3, first half: identifier cap — noted for completeness and already documented in the class javadoc. No change. Local: 626 tests across |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 15 |
🟢 Coverage 94.74% diff coverage
Metric Results Coverage variation Report missing for d463a311 Diff coverage ✅ 94.74% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (d463a31) Report Missing Report Missing Report Missing Head commit (706d86d) 178685 121303 67.89% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5560) 95 90 94.74% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Review:
|
…page 0 updateName is public, has no production caller, and unlike the getIdByName create path did not hold this component's monitor. Documenting that would have left the next reader to discover the consequence the hard way, so serialize it instead: both methods mutate the same entries snapshot, and an append racing the rename would read a half-renamed list. That is the part with no other protection. It deliberately does not make the two atomic on disk, because this method writes inside the caller's transaction and does not commit, so an append committing in between is resolved the usual way by the page version check. The javadoc says exactly that, and that a live rename should also hold a schema-level lock. Also note in the javadoc that the rewrite now dirties every dictionary page in the caller's transaction, so the WAL commit grows with the dictionary rather than being bounded by the single page it used to touch. Log once when page 1 is created, which is the one moment a database stops being readable by a build without multi-page support. It gives an operator a breadcrumb to correlate with a follower later reporting "Dictionary item with id N is not valid". At INFO rather than WARNING: nothing is wrong at that moment, and warnings on healthy growth are how logs stop being read.
|
Round 6. Both javadoc items addressed, one of them with code rather than only a note, and the optional item taken with one adjustment. 1. 2. I was deliberate about what that does and does not buy, and the javadoc now says so: it serialises the in-RAM edit, which is the part with no other protection — an append would otherwise observe a half-renamed list. It does not make the two atomic on disk, because I checked for deadlock before adding it: 3. One-time log when the dictionary rolls over — taking it, with one change: INFO, not WARNING. Nothing is wrong at that moment; rolling over is the healthy behaviour this PR exists to enable, and a warning on normal growth is how logs stop being read. The breadcrumb value you are after is fully preserved at INFO. Triggered on the creation of page 1 specifically, which is once per database lifetime and is exactly the moment downgradability is lost — pages 2 and beyond change nothing about that status, so logging them would be noise. The message names the database and states the consequence and the remedy. 4. Emptied-not-truncated pages — agreed on deferring, for the reasons in the earlier commit message. Local: 626 tests across |
|
Review: feat(engine): let the schema dictionary grow past a single page I read through Dictionary.java, the two dictionary tests, and traced the surrounding paths (TransactionContext.rollback(), PaginatedComponent.getTotalPages() vs pageCount, LocalDatabase.transaction(..., joinCurrentTx), and the BinarySerializer callers). Overall this is a carefully reasoned, exceptionally well-documented change. The core invariant - an id is the ordinal of a name in page order, so the layout must stay strictly append-only across pages - is sound, and the code and comments consistently protect it. No correctness bugs found. WHAT I VERIFIED AND AGREE WITH
MINOR POINTS (non-blocking)
TESTS Coverage is excellent and the test names read as a spec: rollover, id stability across reopen, legacy page-0 read, oversized-name rejection, serializer round-trip across pages, both updateName branches, rolled-back growth, the version guard through the real load path, concurrent appends across a rollover, and the follower applyChanges path. I could not execute mvn in this sandbox (network-restricted), so I relied on the author stated local run (920 engine test classes, 0 failures) plus the static trace above. Nice work - the invariant-first framing and the "why" comments make this unusually easy to review. |
…t rollover The format version is stamped at creation, so a v0 database that later grows past page 0 keeps saying v0. That reads like an oversight, so write down that it is not. Bumping it on first rollover was considered and rejected. The version is part of the file name, so changing it at runtime means renaming a live component file, which PaginatedComponent.rename can only do behind a full flush barrier and which would race replication shipping pages by file id, leaving follower file names diverged from the leader's. And it would protect nothing: ComponentFactory passes the parsed version through without validating it, so a build old enough to be at risk opens a v1 file exactly as happily as a v0 one. The guard added here only helps a newer format meeting this reader. The version therefore records what wrote the file, and the operational signal that a database has left single-page territory stays the INFO line logged when page 1 is created.
|
Round 7. No code changes needed: one point was a question, one is settled by measurement, one was already done. I have documented the answer to the first so it does not have to be re-derived. 1. "Confirm not bumping the on-disk version on first rollover is preferred" — confirmed, and I have written down why rather than leaving it as "intentional", because it does read like an oversight. Two reasons, and the second is the decisive one:
Recorded at 2. Not resizing it either, and here the contrast with the sibling test is the point rather than an inconsistency: 3. Local: 21 dictionary tests, 0 failures. Previous round's wider run was 626 tests across That closes out everything raised across the seven review rounds. Remaining known tradeoffs, all deliberate and documented: emptied-not-truncated pages after an |
Review:
|
The in-RAM name list was a CopyOnWriteArrayList, which copies its whole backing array on every append. Growing to N names therefore cost N^2/2 element copies. That was survivable only because the single page capped the dictionary at ~48K entries, and removing that cap is exactly what this branch does, so the append had to stop being O(n) with it. Measured, appending N names: N CopyOnWriteArrayList doubling array 50,000 232 ms 0 ms 200,000 1,927 ms 0 ms 500,000 11,179 ms 2 ms 4x the names costs 8.3x the time and 2.5x costs 5.8x, which is the quadratic term showing through. A schemaless workload with millions of distinct field names, the case this feature enables, would have spent minutes there. Entries now holds a String[] with spare capacity plus the live size, grown by doubling. An append writes names[size] and then publishes a new Entries with size + 1: sharing the array with older readers is safe because they hold the smaller size and never look at that slot, and a reader that sees the new snapshot sees the slot through the volatile write. Reads stay one volatile read plus an array index, as they were. Swapping a plain ArrayList wholesale under the same volatile field would not have helped: republishing a copy per append is the same O(n) copy the COW list was already doing. The spare capacity is what removes it. updateName renames onto a copy instead of in place. An append only writes slots past the published size so it can share its array; a rename rewrites a slot readers are already looking at, which needs a happens-before edge they do not have. One copy per rename is nothing next to the full page rewrite it precedes. Also fixes the rollover breadcrumb, which fired from inside a transaction that could still roll back: updateName grows inside the caller's transaction, and the rolled-back-growth test rolled back exactly such a growth, so the log announced a rollover that never happened. It now registers an after-commit callback, keyed so several pages in one transaction still log once, and discarded on rollback. A breadcrumb that lies is worse than none. And a test for the compatibility claim in the forward direction, which pageZeroKeepsTheLegacySinglePageLayout only covered in reverse: a single-page dictionary labelled with the previous format version is opened, read in full with no migration, then rolled over past two more pages with every id it arrived with intact.
|
Round 8. All three items addressed in code, including the two you filed as follow-ups — item 1 because you are right that this PR is what makes it matter, so shipping the ceiling removal without it would be shipping the problem. 1. One correction on the proposed remedy, though. "A plain Measured, appending N names:
4x the names costs 8.3x the time and 2.5x costs 5.8x, which is the quadratic term showing through. Your "millions of distinct field names" scenario was heading for minutes of pure array copying.
2. The breadcrumb can fire for growth that never commits — a good catch, and worse than cosmetic in my view: the line's entire value is being trustworthy when correlated against a follower failure, and my own rolled-back-growth test was making it lie. It now registers an after-commit callback via 3b. No forward-direction compatibility fixture — agreed, and a fair gap: 3a. No end-to-end HA test across a real page boundary — agreed this is the sharpest edge, and I am not claiming it covered. Local: 627 tests across |
Review: multi-page schema dictionary (#5560)Reviewed the full 9-commit diff, the final What's strong
Points worth a second look (discussion, not blockers)
Style / conventions
Nice work - the correctness reasoning and test coverage are exemplary. My comments above are refinements, not objections. (Automated review; I was unable to run the build/test suite in this environment, so the reported green run was not independently verified.) |
…issing reload() passed createIfNotExists=true for every page. For page 0 that is load bearing and stays: paired with the Math.max(1, pageCount) floor it materialises page 0 of a file killed mid-write, which is what the single-page reader did. For every later page it was wrong, and worse than the masked corruption it looks like. An invented page is not empty-and-harmless. It contributes zero names, so every name stored after it comes back with an id lower by however many the missing page held, and those ids are embedded in records. Silently renumbering is the one outcome this class exists to prevent, so a gap has to be loud. Later pages now pass createIfNotExists=false and a missing one is reported as a truncated dictionary, naming the page and the count that claimed it. A partial replication replay is how the count gets ahead of the file: applyChanges writes a page and calls updatePageCount(pageNumber + 1) without necessarily having written the pages before it. The new test reproduces that shape directly, in its own database because it deliberately leaves the count inconsistent, and drops it. Also record in the docs how to tell after the fact whether a database has rolled over, since the log line is emitted once and is no help later: the dictionary file is one page per pageSize bytes and pageSize is in its name, so ls answers it.
|
Round 9. Point 2 turned out to be worth more than "flagging", and it produced a real fix. 2. So it is fixed rather than noted. On reachability, since that governs whether the guard is theoretical: 3. The rollover breadcrumb is transient, so there is no durable signal — accepted as stated, but there is a durable one worth naming rather than conceding: the dictionary is one page per 1. 4. The Comment density — noted, and I take the point that it is above the surrounding baseline. Keeping it, per your read, since it is carrying the concurrency and on-disk-format reasoning. Local: 628 tests across Unrelated flake observed, for the record: one full-suite run had |
|
Review: feat(engine): let the schema dictionary grow past a single page (#5560) Thorough, careful piece of work. The single-page dictionary ceiling was a real, permanent wall for schemaless / dynamic-field workloads, and the append-only-across-pages invariant is the right way to lift it without touching the on-disk id encoding. The commit history reads like a design review of itself, and the class Javadoc plus docs/5560-dictionary-multipage.md capture the reasoning the diff alone would hide. I verified the load-bearing claims against the surrounding engine code and they hold up. Notes below, mostly confirmations with a few small points. Correctness (verified)
Minor points
Performance The CopyOnWriteArrayList to doubling-array change is the right fix and the measured quadratic numbers (11s for 500k names) justify it directly; without it, removing the cap would have traded a hard wall for a quadratic one. Reusing a shared backing array across snapshots keeps reads at one volatile read plus an index. DEF_PAGE_SIZE dropping to 65,536 for write-amplification reasons is well argued, and keeping existing DBs on their stored page size is the safe default. Tests Coverage is excellent and each test pins a specific claim rather than the happy path: rollover, reopen survival, legacy page-0 layout in both directions, the too-big-name refusal, the serializer round trip across pages, updateName shrink and grow, rolled-back growth, the version guard through the real load path (via file rename), the truncated-page case in its own throwaway DB, the follower applyChanges path, and a contention test. Exercising the version guard and legacy format through the real load path rather than the constructor is a nice touch. I did not re-run the full engine suite; trusting the stated 920-class clean run. Security No new surface. Only identifiers (create=true callers) enter the dictionary and the per-name size limit is bounded by page size; the Javadoc clarification that user string values never enter is a helpful correction. The name.substring(0, 64) truncation in the error message keeps a pathological identifier from bloating logs. Nice work. The only actionable item is the dead CopyOnWriteArrayList import; the rest are notes. Automated review. Read the full diff and cross-checked PageManager/BasePage; did not execute the test suite. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5560 +/- ##
============================================
- Coverage 66.46% 65.76% -0.71%
- Complexity 0 1113 +1113
============================================
Files 1702 1748 +46
Lines 139704 147059 +7355
Branches 30203 31428 +1225
============================================
+ Hits 92854 96712 +3858
- Misses 34279 37390 +3111
- Partials 12571 12957 +386 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The two halves of the in-RAM view cannot be updated atomically together, so one
leads by an instant and a concurrent reader can land in between. The map was
leading, which is the worse order: that reader resolves a name to an id the
published names do not cover yet, and getNameById() rejects it. Leading with the
names instead means the reader finds the name absent and gets -1, which every
create=false caller already handles, BinarySerializer by skipping the value
compression, and which a create=true caller resolves by blocking on this monitor.
Nobody can be holding the new id before the map publishes it, since it only
reaches a record after getIdByName returns. A miss costs nothing and a throw
costs a request.
Pre-existing ordering, not introduced with the array representation, and the
window is a couple of instructions either way. Closing it was cheaper than
documenting it.
Also drop the CopyOnWriteArrayList import, left behind by the array migration and
kept alive only by a Javadoc link, which unused-import linters flag. The prose
reference stays as {@code}: the reference is historical, so linking the JDK class
bought nothing.
And qualify the wasted-tail claim in the class Javadoc. Under 1% is the append
path; updateName is looser, because a rewrite that shrinks leaves the pages it no
longer reaches empty and appends resume on the last of them, so the ones between
stay empty for good. One zero-entry read each on load, and no production caller.
|
Round 10. The actionable item done, and item 2 closed rather than documented. 1. Dead 2. Map-vs-array ordering window — you asked for a one-line note. I think the better answer is to make the note unnecessary, because the order was the bug, not the absence of documentation. The two halves cannot be updated atomically together, so one leads by an instant either way. What matters is which. Leading with the map, as it did, means the reader in between resolves a name to an id the published names do not cover yet, and So: a miss costs nothing, a throw costs a request. Swapped, with the reasoning recorded at the site. You are right it was pre-existing and a couple of instructions wide either way; closing it was simply cheaper than describing it. Ran the 4-thread contention test three more times plus the full selection, all clean. 3. The "under 1% wasted tail" claim covers the append path, not the post-shrink layout — correct, and the imprecision was mine. Qualified in the class Javadoc, and it is slightly worse than you described: after a shrink, appends resume on the last page, which is one of the emptied ones, so every page between the last used one and that tail stays empty for good, not just the one partial tail. They cost one zero-entry read each on load and nothing else, and Local: 628 tests across Standing item, unchanged from the previous round: there is still no end-to-end |
Review:
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 17 |
🟢 Coverage 95.05% diff coverage
Metric Results Coverage variation Report missing for d463a311 Diff coverage ✅ 95.05% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (d463a31) Report Missing Report Missing Report Missing Head commit (cd0f669) 179051 121584 67.90% Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#5560) 101 96 95.05% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
The rollover is logged once, when it happens, which is no help to someone looking at a database afterwards or planning a rollback. Log the same fact as current state at open time instead, when the dictionary already spans more than one page, naming the page count. Preferred over the persisted marker the review floated: the page count already is the durable record, so a separate marker would be a second copy of the same fact that can drift from it. A startup compatibility check cannot help either, because the build that needs the warning is the older one, which predates the concept and cannot be given a check retroactively. Restating the state on every open is what actually survives log rotation.
|
Round 11. Item 1's premise needs a correction, item 2 is addressed, and I ran the verification you asked for. 1. Public API removal of 2. The rollover signal is only a one-time INFO, easily missed after the fact — this is the third round to land on the operational hazard, and the "easily missed after the fact" part is the fair core of it. Addressed: the dictionary now logs its state on every open when it already spans more than one page, naming the page count. I deliberately did not take either of the two mechanisms you suggested:
Restating current state at open is what actually survives log rotation, which was the real complaint. The rollover event line stays as well, so an operator gets it both as it happens and on every subsequent start. Test note: confirm 3, 4, 5 — agreed as stated, no changes. The On the red CI checks, since they matter more than the notes
Local: 628 tests across |
Review: multi-page schema dictionary (#5560)Reviewed Correctness / concurrency
Minor
TestsCoverage is excellent and the intent comments make each case easy to follow: rollover, reopen persistence, legacy page-0 layout in both directions, serializer round-trip across pages, Nice work - the append-only invariant and the "make failure loud rather than silently renumber" stance are exactly right for on-disk ids embedded in records. |
|
Correction to my CI note above. In my last comment I said The reporter is not broken. It is correctly reporting a genuinely failing integration test: What misled me is that the Maven step runs with What stands from my earlier note: this is not a regression from this PR. Run I am picking this up separately along with the other follow-ups. |
…nise The index arithmetic walked record * (PROPERTIES / TYPE_RECORDS) + p, which only covers 0..PROPERTIES-1 without gaps while the two constants divide evenly. At 400/5 they do, but a later edit to a non-divisible pair would truncate, write fewer distinct names than the follower loop then looks for, and report missing names that were never written - a confusing failure in a test whose whole job is to be trusted about missing names. Documenting the constraint was the suggestion; making it unrepresentable is better. PROPERTIES is now derived as TYPE_RECORDS * PROPERTIES_PER_RECORD, so there is no pair of numbers a future editor can pick that desynchronises them, and the arithmetic uses the per-record constant directly. Also stop abbreviating the key in the assertion label: it said "p" + p while the real key is the padded 500-character name, so grepping the message for the key found nothing. It now reads as an index, which is what it is.
#5560 removed the single-page cap on the schema dictionary and merged before the living release notes file existed, so it is not in them. Add it under New Features, in the style of the entries already there. The part that needs to reach an operator rather than a reader: in a cluster, followers have to be upgraded before or together with the leader. Dictionary pages replicate as raw pages, so a rolled-over leader ships page 1 and beyond, and an older follower writes them but reloads only page 0 - leaving it missing every name past the first page and failing each record that references one. The entry also gives the ls recipe for telling whether a given database has rolled over, since that decides whether it can still be opened by an older build.
The getTotalPages() > 1 guard assumes 400 names padded to 500 bytes overflow page 0, which is an implicit dependency on the dictionary page size that a reader had to rederive. Say it: DEF_PAGE_SIZE leaves roughly 65Kb usable per page and the names are about 200Kb, so the boundary is crossed around three times over rather than only just, and if the default ever grew past the total written the guard fails loudly rather than passing without a rollover. Comment only, no behaviour change. Verified by running the test through the same profile CI uses: it passes, and with reload() pinned back to a single page it fails on property #130 of a follower - which is where page 0 fills at this name length, so it still fails for the reason it exists.
…xture is empty Two comment nits from review. Neither changes behaviour. The page-size note said "roughly 65Kb usable per page", which review read as overstating a DEF_PAGE_SIZE of 65,536 bytes - 64Kb binary, 65.5Kb decimal, and the ambiguity is the whole problem. Swapping one Kb for another just moves it, so the figures are now in bytes: 65,536 per page less the headers, against about 200,000 bytes of names. Nothing to misread. populateDatabase() is overridden empty on purpose and did not say so. The base fixture builds several types with properties, indexes and records, and every one of those names reaches the dictionary before the test starts; this test opens by asserting a single page, so the emptiness is what makes that assertion mean anything. Said so. Re-ran the test through the profile CI uses: passes in 21.9s.
…e of them The comment over the follower loop said resolving the name is what fails when a follower stopped at page 0, which implies resolving the id would still work. It would not: such a follower holds neither mapping for a name past the boundary, so getIdByName returns -1 and the missing-id assertion trips first. The pinned-page run says as much - it fails on "property #130 must be in the dictionary", not on the read-back. Reworded to say both break together and that the missing-id assertion is simply the one reached first, while keeping the point the original was reaching for: the id->name direction is the one a real record read hits, which is why the read-back further down is the user-visible end of the same failure. Comment only. Re-ran through the profile CI uses: passes in 21.4s.
…ary (#5563) * test(ha-raft) #5560: replicate a dictionary that crosses a page boundary The multi-page dictionary shipped with the follower path covered only by a unit test that fed a hand-built WALFile.WALPage to applyChanges. That checks the reload walks every page, but not the scenario the upgrade-ordering requirement in docs/5560-dictionary-multipage.md is about: a real leader crossing the boundary and real followers having to use what arrives. Three nodes. The leader writes enough distinct property names that ordinary inserts push the dictionary off page 0, which is how it happens in production rather than by calling the dictionary directly. Then every server has to report more than one page, resolve all 400 names in both directions, and read every record back - the last one being the user-visible symptom, since property names live inside records as dictionary ids and deserialising them is what turns a missing page into "Dictionary item with id N is not valid". Verified it detects the regression: with reload() pinned back to a single page it fails, and passes with the page walk restored. Tagged slow at 19s. * test(ha-raft) #5560: derive the property count so it cannot desynchronise The index arithmetic walked record * (PROPERTIES / TYPE_RECORDS) + p, which only covers 0..PROPERTIES-1 without gaps while the two constants divide evenly. At 400/5 they do, but a later edit to a non-divisible pair would truncate, write fewer distinct names than the follower loop then looks for, and report missing names that were never written - a confusing failure in a test whose whole job is to be trusted about missing names. Documenting the constraint was the suggestion; making it unrepresentable is better. PROPERTIES is now derived as TYPE_RECORDS * PROPERTIES_PER_RECORD, so there is no pair of numbers a future editor can pick that desynchronises them, and the arithmetic uses the per-record constant directly. Also stop abbreviating the key in the assertion label: it said "p" + p while the real key is the padded 500-character name, so grepping the message for the key found nothing. It now reads as an index, which is what it is. * test(ha-raft) #5560: record the page size the fixture is sized against The getTotalPages() > 1 guard assumes 400 names padded to 500 bytes overflow page 0, which is an implicit dependency on the dictionary page size that a reader had to rederive. Say it: DEF_PAGE_SIZE leaves roughly 65Kb usable per page and the names are about 200Kb, so the boundary is crossed around three times over rather than only just, and if the default ever grew past the total written the guard fails loudly rather than passing without a rollover. Comment only, no behaviour change. Verified by running the test through the same profile CI uses: it passes, and with reload() pinned back to a single page it fails on property #130 of a follower - which is where page 0 fills at this name length, so it still fails for the reason it exists. * test(ha-raft) #5560: say the page arithmetic in bytes, and why the fixture is empty Two comment nits from review. Neither changes behaviour. The page-size note said "roughly 65Kb usable per page", which review read as overstating a DEF_PAGE_SIZE of 65,536 bytes - 64Kb binary, 65.5Kb decimal, and the ambiguity is the whole problem. Swapping one Kb for another just moves it, so the figures are now in bytes: 65,536 per page less the headers, against about 200,000 bytes of names. Nothing to misread. populateDatabase() is overridden empty on purpose and did not say so. The base fixture builds several types with properties, indexes and records, and every one of those names reaches the dictionary before the test starts; this test opens by asserting a single page, so the emptiness is what makes that assertion mean anything. Said so. Re-ran the test through the profile CI uses: passes in 21.9s. * test(ha-raft) #5560: the two lookup directions break together, not one of them The comment over the follower loop said resolving the name is what fails when a follower stopped at page 0, which implies resolving the id would still work. It would not: such a follower holds neither mapping for a name past the boundary, so getIdByName returns -1 and the missing-id assertion trips first. The pinned-page run says as much - it fails on "property #130 must be in the dictionary", not on the read-back. Reworded to say both break together and that the missing-id assertion is simply the one reached first, while keeping the point the original was reaching for: the id->name direction is the one a real record read hits, which is why the read-back further down is the user-visible end of the same failure. Comment only. Re-ran through the profile CI uses: passes in 21.4s.
Important
Release note / upgrade requirement: in a cluster, upgrade followers before, or together with, the leader.
Dictionary pages replicate as raw pages. Once a leader's dictionary grows past its first page it ships page 1 and
beyond to its followers, and a follower still running a build without multi-page support writes those pages but
reloads only page 0. Its in-RAM dictionary is then missing every name past the first page, and each record
referencing one fails with
Dictionary item with id N is not valid.Related: a database that has rolled over can no longer be opened by an older ArcadeDB at all. To check whether a
given database has rolled over, divide its dictionary file size by the page size in its own file name:
ls -l <db>/dictionary.*.dict- 131072 / 65536 = 2 pages means rolled over. A file at or under one page is stillsingle-page and remains downgradable.
Full detail in
docs/5560-dictionary-multipage.md, section Upgrade notes.The limit
com.arcadedb.engine.Dictionarymaps every type name, property name and enumerated string value to a small integer id, and that id is what records carry instead of the name. Despite the class javadoc saying "CONTENT-PAGES", it only ever used page 0, so a database was capped at whatever fitted there:pageSize - 8 - 4= 327,668 bytes of names, measured at 48,396 short names before the write was refused.Past that point
CREATE PROPERTY, or inserting a document with a new field name, failed permanently. There was no path to grow, and entries are never reclaimed, so a schemaless workload with dynamic field names walked steadily toward it.What changed
Names roll over to a new page. Every page, page 0 included, carries the same 4-byte legacy counter, so a dictionary written before this change is exactly a dictionary of one page and loads unchanged: no migration, no rewrite, no format flag needed to read an existing database.
The invariant everything follows from: an id is the ordinal of the name in page order, and that id sits inside records on disk. So the layout is strictly append-only across pages. Names go only to the LAST page, and a page left behind is never revisited even when it still has room. Filling a gap would renumber every name after it and silently repoint every record referencing them, which is the one way this change could corrupt data rather than throw. A name is never split across pages, so the wasted tail is under 1% at identifier lengths, and the only limit left is a single name larger than one page, which is now refused by name.
Three details worth review attention:
addItemToPagereads the tail page before deciding where to write. Asking for it as modifiable and then finding it full would bump its version, rewrite it at commit for nothing, and make it false-conflict with concurrent transactions.reload()walks the committed page count, notgetTotalPages().TransactionContext.rollback()callsreload()while the transaction's own page counter still counts pages being thrown away. The floor of 1 keeps the previous behaviour of always reading page 0 of a non-empty file.updateName()re-lays the dictionary out from page 0 in the same order, so no id moves, and empties the pages the new content no longer reaches.reload()walks every committed page, so a stale tail page would otherwise re-add its old names.Page size and format version
DEF_PAGE_SIZEdrops from 327,680 to 65,536 for new dictionaries. That size existed only to make the single available page hold as many names as possible. Now that pages roll over, a new name dirties and eventually flushes one page, so a smaller page is 5x less write amplification per name. Existing databases keep the page size they were created with, which is read back from the file name.CURRENT_VERSIONgoes to 1, with a guard refusing a dictionary written by a newer ArcadeDB rather than misreading it.Downgrade note: once a database grows past page 0 it can no longer be opened by an older ArcadeDB, which reads page 0 only and reports
Dictionary item with id N is not valid. Loud, not silent, and such a database could not have existed before this change since the write would have been refused.Tests
New
DictionaryMultiPageTest(incom.arcadedb.engine, so it can inspect the on-page layout):updateNamerewrites every page and keeps every other idDictionaryLimitsTesthad a test asserting the old "no space left in dictionary file" behaviour; it now asserts that filling well past one page keeps working.Local run: 920 engine test classes, 0 failures, at the time of pushing (the full run was still going through an unrelated long test).