Remove non-transactional sequence state, and disallow non-default sequence params - #5880
Remove non-transactional sequence state, and disallow non-default sequence params#5880gefjon wants to merge 7 commits into
Conversation
…uence params Our sequences code had a lot of complexity that wasn't paying for itself, and was causing bugs. This commit simplifies it in two ways: 1. Sequences are no longer configurable. All sequences now have a step of 1 and a range of `1..=i128::MAX`, all user sequences start at 1, and all system table sequences start just after the reserved range. Even prior to this change, sequences were only used to implement `auto_inc` columns, and `auto_inc` columns all configured their sequences in exactly this way. It was technically possible for a module which entirely bypassed our APIs, and emitted a `RawModuleDefV10` on its own, to configure a sequence otherwise, but we have no reason to believe anyone did this and do not consider it supported. 2. Sequences no longer pre-allocate chunks of values, and instead just update their `st_sequence` rows directly. Prior to this change, we kept an in-memory non-transactional side table of sequence states. This is a common optimization in concurrent databases with MVCC datastores, as it prevents contention on the system table rows which implement the sequences persistently. However, SpacetimeDB is not a concurrent database and does not have an MVCC datastore, so contention is not an issue. This change was brought on by a bug wherein a transaction could allocate a new block of sequence values and then roll back. This left the in-memory side table advanced but the persistent `st_sequence` row not updated, which, when using a sequence to implement the `auto_inc` portion of an `auto_inc` unique column, led to unique constraint violations when the database restarted and then re-allocated the same block of values. Abandoning the in-memory side table entirely is sufficient (though not necessary) to prevent this bug.
I even ran it against the pre-change version and saw it fail!
We had a few unit tests that contained assertions about sequences' previous behavior, where rolled back transactions could consume auto-incremented values. Rolled back transactions cannot consume auto-incremented values anymore, so these tests change slightly.
| /// so that tests which rely on auto-inc sequences will be deterministic. | ||
| /// See [`CommittedState::sequence_advance_simulate_reallocation_rng`]. | ||
| fn should_simulate_sequence_reallocation(rng: &mut Xoshiro128PlusPlus) -> bool { | ||
| // Skip an average of once every 4096 values, the same as the chunk size. |
There was a problem hiding this comment.
It seems like we will skip an average of 2048 steps out of every 4096 allocations. That feels like a large portion of the range to skip, and a low enough frequency that it would be easy for testing to not hit it. What do you think about something like, add one extra step every 4 allocations (on average)?
There was a problem hiding this comment.
I did introduce a test that hit the skip, though it did require quite a few inserts to hit the case. My concern with a smaller skip is that it won't be noticeable to users. I wanted to pick a skipping behavior where users will be easily able to tell that it's happening just by looking at, say, a list of values, and so I wanted both a large gap and to skip to an easily identifiable round number.
There was a problem hiding this comment.
I suppose we could consider more closely emulating the previous behavior: don't do random skips during operation, but do skip ahead when restarting.
| .unwrap(); | ||
| let old_seq_row_ptr = old_seq_row_ref.pointer(); | ||
| let seq_row = { | ||
| let (seq_row, value) = { |
There was a problem hiding this comment.
We could just use seq_row as the value to simplify a little bit. The reallocation case would just bump the allocation one step sooner.
There was a problem hiding this comment.
You mean, later on when we read value, we could instead read seq_row.allocated - 1? We could do that, and it would use fewer lines of code, but I don't particularly see it as simpler.
| /// The supplied `rng` should be the one in the committed state for this purpose, | ||
| /// so that tests which rely on auto-inc sequences will be deterministic. | ||
| /// See [`CommittedState::sequence_advance_simulate_reallocation_rng`]. | ||
| fn should_simulate_sequence_reallocation(rng: &mut Xoshiro128PlusPlus) -> bool { |
There was a problem hiding this comment.
Is this using a different rng than the one that is used by modules?
There was a problem hiding this comment.
Yes, this uses a new PRNG held by the CommittedState just for this purpose. Unlike the one used by modules:
- It lives in the host space, not within the module VM.
- It persists across transactions.
- It is not a cryptographic PRNG, it is instead a lightweight and fast algorithm.
- It is seeded directly from the OS's RNG source (except in
#[cfg(test)], where it instead has a stable seed).
2 participants
Description of Changes
Our sequences code had a lot of complexity that wasn't paying for itself, and was causing bugs. This commit simplifies it in two ways:
Sequences are no longer configurable. All sequences now have a step of 1 and a range of
1..=i128::MAX, all user sequences start at 1, and all system table sequences start just after the reserved range. Even prior to this change, sequences were only used to implementauto_inccolumns, andauto_inccolumns all configured their sequences in exactly this way. It was technically possible for a module which entirely bypassed our APIs, and emitted aRawModuleDefV10on its own, to configure a sequence otherwise, but we have no reason to believe anyone did this and do not consider it supported.Sequences no longer pre-allocate chunks of values, and instead just update their
st_sequencerows directly. Prior to this change, we kept an in-memory non-transactional side table of sequence states. This is a common optimization in concurrent databases with MVCC datastores, as it prevents contention on the system table rows which implement the sequences persistently. However, SpacetimeDB is not a concurrent database and does not have an MVCC datastore, so contention is not an issue.This change was brought on by a bug wherein a transaction could allocate a new block of sequence values and then roll back. This left the in-memory side table advanced but the persistent
st_sequencerow not updated, which, when using a sequence to implement theauto_incportion of anauto_incunique column, led to unique constraint violations when the database restarted and then re-allocated the same block of values. Abandoning the in-memory side table entirely is sufficient (though not necessary) to prevent this bug.In order to prevent users from relying on the determinism and sequentiality of the new implementation, we use a PRNG to artificially skip values occasionally when reading from a sequence.
API and ABI breaking changes
The
RawModuleDefABI family (V8, V9 and V10) have their layout preserved, but the validation pass is now more restrictive than it was previously, and rejectsSequenceDefs which would previously have been rejected. Modules defined using the SpacetimeDB module libraries (Rust, TypeScript, C# and C++) would never exercise this difference, and will not be rejected.Rollback safety impact
n/a
Expected complexity level and risk
3: it may be somewhat difficult to convince ourselves that:
Testing
st_index'sindex_idcolumn.autoinc_sequence_allocation_remains_consistent_after_rollback, which fails on master but passes on this branch.sequence_occasionally_skips_values_to_simulate_reallocation, which demonstrates that we occasionally skip values when advancing a sequence in order to simulate block allocation.