Skip to content

Integrate tiered storage - #6305

Draft
kim wants to merge 31 commits into
kim/tiered-storage-basefrom
kim/tiered-storage-integration
Draft

kim wants to merge 31 commits into
kim/tiered-storage-basefrom
kim/tiered-storage-integration

Conversation

@kim

@kim kim commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

Description of Changes

Integrates tiered storage into the rest of the codebase.

This is a big change, because every call site that may access a page or blob
becomes fallible due to the possibility of fault errors.

Much of this is mechanical, but note that transaction commit becomes a two-step
process with this patch: first, all pages and blobs affected by the transaction
are faulted in if necessary, and pinned so they can't be evicted until the
transaction completes. Then, the transaction is actually applied -- this remains
infallible and therefore atomic.

API and ABI breaking changes

Internal only.

Rollback safety impact

Currently rollback safe.

Expected complexity level and risk

5

Testing

@kim
kim added this pull request to stack #6306 October 15, 2026 14:44
joshua-spacetime added a commit that referenced this pull request Oct 16, 2026
Remove nine newly added Deep storage tests (429 lines) at the user's request to reduce the review diff. This removes new coverage for resident/tiered storage, prepared inserts, page reuse, and blob pins. Existing tests from #6305 and all production code remain unchanged.

Validation: 112 existing spacetimedb-table library tests pass; clippy for the library and tests passes with -D warnings.
joshua-spacetime added a commit that referenced this pull request Oct 16, 2026
Remove the local macOS benchmarking accommodations from the review diff. Restore the original unconditional Table=328 assertion and keep TxState=144, which accounts for the inline transaction blob map. Non-macOS compiled code is unchanged.

Native macOS builds again fail the original size checks, as on #6305; a compatibility patch is saved outside the branch for future local profiling.

Validation: rustfmt and git diff --check; exact diff is six deleted lines. Native clippy confirms the expected Table size assertion failure (328 versus 336). Linux checks were not rerun locally.
@kim
kim force-pushed the kim/tiered-storage-integration branch from 7625b27 to b840802 Compare October 19, 2026 06:31
Comment thread crates/table/src/bflatn_to.rs Outdated
Comment on lines +161 to +167
serialized.write_large_blobs(blob_store)
serialized.write_large_blobs(blobs)?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to rollback previous changes to the page now that this is fallible, no?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ya possibly. I think it is actually infallible because we pinned all the blobs, but that is not expressible locally at the moment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines +727 to +729
// All page mutations have finished, so pinned blob frames no longer
// need to be pinned by the commit guard.
drop(prepared.pinned_blobs);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This releases the blob pins, but where do we decrement the ref count and eventually drop/remove the blob(s)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refcounts are in the BlobSet that is owned by the transaction state, while the blob frames are shared via the ResidentObjects. The normal insertion path then updates the refcount in the committed state's BlobSet.

Just dropping the handles will leave dangling weak references in ResidentObjects, unless successful insertion created new strong references in the committed state BlobSet (they are guaranteed to become resident, because we pinned them).

Comment thread crates/table/src/table.rs Outdated
Comment on lines +1597 to +1851
ptrs.extend(self.scan_rows(&NullBlobStore).map(|row| row.pointer()));
ptrs
for row in self.scan_rows(&BlobSet::new_for_test()) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this equivalent to NullBlobStore?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it's not, the NullBlobStore panics on any method invocation. I'll have to find a way to emulate this, as BlobSet is not a trait object.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread crates/table/src/table.rs Outdated

// If we removed the last unique index, add a pointer map.
if index.is_unique() && !self.indexes.values().any(|idx| idx.is_unique()) {
self.pointer_map = Some(pointer_map.unwrap_or_else(|| self.rebuild_pointer_map(blob_store)));
self.pointer_map = Some(pointer_map.map_or_else(|| self.rebuild_pointer_map(blobs), Ok)?);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When do we record a pending schema change for a deleted index? Before or after this call? If after, we'll rollback to an invalid state if the pointer map rebuild fails.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally before. I added rollback_add_index specifically to not rebuild the pointer map on rollback -- we should end up in the same state that we started with as pending schema changes get reverted in reverse order (and rebuilding on intermediate reverts was probably a bug to begin with). The TODO is to remind me that this needs a test.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment thread crates/table/src/table.rs Outdated
@@ -1377,33 +1610,34 @@ impl Table {
// SAFETY:
// - `self` trivially has the same `row_layout` as `self`.
// - We just inserted `temp_ptr`, so it's valid.
let (_, existing_row_ptr) = unsafe { Self::find_same_row(self, self, blob_store, temp_ptr, None) };
let (_, existing_row_ptr) = unsafe { Self::find_same_row(self, self, blobs, temp_ptr, None) }?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The temporary insert above needs to be rolled back in the case of a failure here as well as the other fallible calls below.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see at moment how this method could be made atomic, because deleting the temporary row can itself fail. But what should happen then?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joshua-spacetime joshua-spacetime left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More rollback safety concerns. It would be good to add some rollback tests if they're indeed legitimate.

Comment on lines +804 to +823
let effects = UpdateEffects::after_migration(res, &tx);
let effects = UpdateEffects::after_migration(res, &tx)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback safety: If this fails, do we undo the migration?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 2685 to 2689

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback safety: The revert is skipped if get_row_ref fails.

Comment on lines +2731 to +2732
commit_table.make_index_non_unique(index_id, commit_blob_store)?;
tx_table.make_index_non_unique(index_id, tx_blob_store)?;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rollback safety: make_index_non_unique succeeds for commit_table but fails on tx_table. Is this a problem? It would appear so, since we haven't added the pending schema change at this point.

@kim
kim force-pushed the kim/tiered-storage-integration branch from 8c3a919 to 4a229fb Compare October 22, 2026 16:10
@kim
kim removed this pull request from stack #6306 October 22, 2026 16:12
@kim
kim added this pull request to stack #6360 October 22, 2026 16:21
@kim
kim force-pushed the kim/tiered-storage-integration branch 5 times, most recently from f9e3421 to 89a9a07 Compare October 23, 2026 22:51
@kim
kim force-pushed the kim/tiered-storage-integration branch from 89a9a07 to 002db2f Compare October 23, 2026 23:06
joshua-spacetime added a commit that referenced this pull request Oct 29, 2026
Remove nine newly added Deep storage tests (429 lines) at the user's request to reduce the review diff. This removes new coverage for resident/tiered storage, prepared inserts, page reuse, and blob pins. Existing tests from #6305 and all production code remain unchanged.

Validation: 112 existing spacetimedb-table library tests pass; clippy for the library and tests passes with -D warnings.
joshua-spacetime added a commit that referenced this pull request Oct 29, 2026
Remove the local macOS benchmarking accommodations from the review diff. Restore the original unconditional Table=328 assertion and keep TxState=144, which accounts for the inline transaction blob map. Non-macOS compiled code is unchanged.

Native macOS builds again fail the original size checks, as on #6305; a compatibility patch is saved outside the branch for future local profiling.

Validation: rustfmt and git diff --check; exact diff is six deleted lines. Native clippy confirms the expected Table size assertion failure (328 versus 336). Linux checks were not rerun locally.
@joshua-spacetime
joshua-spacetime force-pushed the kim/tiered-storage-integration branch 2 times, most recently from 1e739db to bb400af Compare October 30, 2026 20:00
kim and others added 5 commits October 30, 2026 17:14
Integrates tiered storage, replacing `Pages` with `PageSet` and
`BlobStore` with `BlobSet`. Handles fallible object acquisition
everywhere.

Also implements two-phase transaction commit, which preflights the
transaction and pins all affected objects. This allows the actual merge
into the committed state to be infallible and therefore atomic.
Keep directly owned pages and borrowed access for resident tables while
retaining tiered page eviction and the newer RowRef API. Stream eligible
resident commits while their transaction state is alive, reuse resident
pages and budget permits, and reduce row/blob bookkeeping overhead.

Also fix shared blob registration and preserve logical blob entries on
eviction, with coverage for both backends, budgets, snapshots and pins.

Validation: 239 storage tests pass; Clippy passes with -D warnings.
Three alternating 60-second TypeScript keynote runs with pipelining and
confirmed reads enabled averaged 231,612 transfers/sec versus 241,902
on master (4.25% lower; paired gaps 3.26%, 3.49%, and 5.99%).
Share preparation, rollback on failure, and application in
CommittedState::merge, remove the prepare_merge forwarding wrapper, and
reuse one helper for recycling empty resident pages with their permits.
Retain all existing error boxing, inlining, and resident/tiered fast paths.

Validation: 239 storage tests pass; Clippy passes with -D warnings.
Three alternating 60-second TypeScript keynote pairs with pipelining and
confirmed reads averaged 231,693 transfers/sec versus 234,454 before
this cleanup (-1.18% in this batch). All account checks pass.
Remove nine newly added Deep storage tests (429 lines) at the user's request to reduce the review diff. This removes new coverage for resident/tiered storage, prepared inserts, page reuse, and blob pins. Existing tests from #6305 and all production code remain unchanged.

Validation: 112 existing spacetimedb-table library tests pass; clippy for the library and tests passes with -D warnings.
Remove the local macOS benchmarking accommodations from the review diff. Restore the original unconditional Table=328 assertion and keep TxState=144, which accounts for the inline transaction blob map. Non-macOS compiled code is unchanged.

Native macOS builds again fail the original size checks, as on #6305; a compatibility patch is saved outside the branch for future local profiling.

Validation: rustfmt and git diff --check; exact diff is six deleted lines. Native clippy confirms the expected Table size assertion failure (328 versus 336). Linux checks were not rerun locally.
joshua-spacetime and others added 26 commits October 30, 2026 17:14
Format modified Rust files with the repository rustfmt configuration used by cargo ci lint. Update the unconditional Table size assertion from 328 to the 336 bytes reported by Linux portable-datastore CI. No macOS-specific branches or runtime logic are added.

Validation: exact CI rustfmt check passes for all 1,611 tracked Rust files; table/datastore clippy (lib, tests, benches; -D warnings) and 15 portable datastore unit tests pass locally. Local macOS checks used a temporary TxState=152 assertion patch, removed before committing; branch retains TxState=144.
Document resident versus tiered ownership, allocation-free empty blob pins, the direct commit eligibility and transaction lifetime, cached page permit accounting, inline blob-map ownership, resident reads/scans/deletion, iterator inlining, and boxed error sizes. Keep the explanations next to the relevant implementation without numbering them against the PR description.

Comment-only change: no executable Rust code changed. Exact CI rustfmt check passes for all 1,611 tracked Rust files; table/datastore clippy for lib, tests, and benches passes with -D warnings. Native checks used the temporary local TxState size patch, restored before committing.
assert that an existing frame is actually the same allocation.
Commit changes only after potential pointer map rebuild succeeded.
If the pointer map needs to be rebuilt, rebuild it first before
committing the changes.
This prevents unnecessarily faulting in blobs.

With tiered storage, constructing a RowRef requires to load a
BlobReadSet, so calling `scan_rows` with an empty BlobSet is not
equivalent to the previous `NullBlobStore`.
Fallibly collect blob references before mutating anything.
Delete rows by marking all pages as absent.
`MutTxId::delete_by_row_value`, too
To make this possible, introduce a `PinnedRow` that works like an owned
`RowRef`. When a pinned row is in scope, potentially faulting functions
can be unwrapped, as they are guaranteed to not fault the pinned row's
page.
@joshua-spacetime
joshua-spacetime force-pushed the kim/tiered-storage-integration branch from bb400af to 8eb4f30 Compare October 31, 2026 00:14

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants