[alpha-playtest-3]: [alpha-playtest-3]: Cherrypick WIP PR #1532: Don'…

…t pre-allocate seqs during bootstrap; fix off-by-ones when allocating.

Also use `pretty_assertions` in some tests,
because I was having trouble debugging small differences in large structures.
Notably does not use `pretty_assertions` in our whole test suite,
only in the tests broken by the previous commits in this PR.

Prior to this commit, we pre-allocated 4096 values for each sequence during bootstrap.
For user sequences, these were 0..=4096; for system sequences, they were 4097..=8192.
This did not play nicely with restoring after a restart;
we would either incorrectly re-use values starting from 4097 after restart,
or would spuriously allocate after bootstrap without a restart
and begin with values starting from 8193.

With this commit, we do not pre-allocate sequence values during bootstrap.
Each user table sequence starts with `value: 1, allocation: 0`,
and each system sequence with `value: 4097, allocation: 4096`.
This means that the first access to a sequence, either after bootstrap or after a restart,
will perform an allocation.
This is in contrast to previously, where accesses after restart would allocate,
but accesses after bootstrap would not.

Additionally, the logic for determining whether an allocation was necessary
in `MutTxId::get_next_sequence_value` contained an off-by-one value
which caused the last value before an allocation to be skipped.
This commit fixes that off-by-one error, making it so that yielding value `4096`
when `allocation == 4096` is possible, though it does result in a new allocation.
Previously, we would yield 4095 without allocation, skip 4096,
then allocate and yield 4097.

Cargo.toml

@@ -171,6 +171,7 @@ parking_lot = { version = "0.12.1", features = ["send_guard", "arc_lock"] }
 paste = "1.0"
 pin-project-lite = "0.2.9"
 postgres-types = "0.2.5"
+pretty_assertions = "1.4"
 proc-macro2 = "1.0"
 prometheus = "0.13.0"
 proptest = "1.4"

crates/core/Cargo.toml

@@ -118,6 +118,7 @@ proptest.workspace = true
 proptest-derive.workspace = true
 rand.workspace = true
 env_logger.workspace = true
+pretty_assertions.workspace = true
 
 [build-dependencies]
 prost-build.workspace = true

crates/core/src/db/datastore/locking_tx_datastore/committed_state.rs

@@ -223,9 +223,9 @@ impl CommittedState {
                 // All sequences for system tables start from the reserved
                 // range + 1.
                 // Logically, we thus have used up the default pre-allocation
-                // and must initialize the sequence with twice the amount.
+                // and must allocate again on the next increment.
                 start: ST_RESERVED_SEQUENCE_RANGE as i128 + 1,
-                allocated: (ST_RESERVED_SEQUENCE_RANGE * 2) as i128,
+                allocated: ST_RESERVED_SEQUENCE_RANGE as i128,
             };
             let row = ProductValue::from(row);
             // Insert the meta-row into the in-memory ST_SEQUENCES.

crates/core/src/db/datastore/locking_tx_datastore/datastore.rs

@@ -917,6 +917,7 @@ mod tests {
     use crate::db::datastore::Result;
     use crate::error::{DBError, IndexError};
     use itertools::Itertools;
+    use pretty_assertions::assert_eq;
     use spacetimedb_lib::address::Address;
     use spacetimedb_lib::error::ResultTest;
     use spacetimedb_primitives::{col_list, ColId, Constraints};
@@ -1124,7 +1125,7 @@ mod tests {
                 start: value.start,
                 min_value: 1,
                 max_value: 170141183460469231731687303715884105727,
-                allocated: 4096,
+                allocated: 0,
             }
         }
     }
@@ -1140,7 +1141,7 @@ mod tests {
                 start: value.start,
                 min_value: 1,
                 max_value: 170141183460469231731687303715884105727,
-                allocated: 4096,
+                allocated: 0,
             }
         }
     }
@@ -1358,7 +1359,7 @@ mod tests {
                 SequenceRow { id: 2, table: 4, col_pos: 0, name: "seq_st_constraints_constraint_id_primary_key_auto", start },
             ],
             |row| StSequenceRow {
-                allocated: ST_RESERVED_SEQUENCE_RANGE as i128 * 2,
+                allocated: ST_RESERVED_SEQUENCE_RANGE as i128,
                 ..StSequenceRow::from(row)
             }
         ));

crates/core/src/db/datastore/locking_tx_datastore/mut_tx.rs

@@ -30,7 +30,7 @@ use spacetimedb_sats::{
         auth::StAccess,
         def::{
             ConstraintDef, ConstraintSchema, IndexDef, IndexSchema, SequenceDef, SequenceSchema, TableDef, TableSchema,
-            SEQUENCE_PREALLOCATION_AMOUNT,
+            SEQUENCE_ALLOCATION_STEP,
         },
         error::SchemaErrors,
     },
@@ -473,9 +473,11 @@ impl MutTxId {
                 return Err(SequenceError::NotFound(seq_id).into());
             };
 
-            // If there are allocated sequence values, return the new value, if it is not bigger than
-            // the upper range of `sequence.allocated`
-            if let Some(value) = sequence.gen_next_value().filter(|v| v < &sequence.allocated()) {
+            // If there are allocated sequence values, return the new value.
+            // `gen_next_value` internally checks that the new allocation is acceptable,
+            // i.e. is less than or equal to the allocation amount.
+            // Note that on restart we start one after the allocation amount.
+            if let Some(value) = sequence.gen_next_value() {
                 return Ok(value);
             }
         }
@@ -493,13 +495,19 @@ impl MutTxId {
             let Some(sequence) = self.sequence_state_lock.get_sequence_mut(seq_id) else {
                 return Err(SequenceError::NotFound(seq_id).into());
             };
-            seq_row.allocated = sequence.nth_value(SEQUENCE_PREALLOCATION_AMOUNT as usize);
+            seq_row.allocated = sequence.nth_value(SEQUENCE_ALLOCATION_STEP as usize);
             sequence.set_allocation(seq_row.allocated);
             seq_row
         };
 
         self.delete(ST_SEQUENCES_ID, old_seq_row_ptr)?;
-        self.insert(ST_SEQUENCES_ID, &mut ProductValue::from(seq_row), database_address)?;
+        // `insert_row_internal` rather than `insert` because:
+        // - We have already checked unique constraints during `create_sequence`.
+        // - Similarly, we have already applied autoinc sequences.
+        // - We do not want to apply autoinc sequences again,
+        //   since the system table sequence `seq_st_table_table_id_primary_key_auto`
+        //   has ID 0, and would otherwise trigger autoinc.
+        self.insert_row_internal(ST_SEQUENCES_ID, &ProductValue::from(seq_row))?;
 
         let Some(sequence) = self.sequence_state_lock.get_sequence_mut(seq_id) else {
             return Err(SequenceError::NotFound(seq_id).into());

crates/core/src/db/datastore/locking_tx_datastore/sequence.rs

@@ -86,7 +86,9 @@ impl Sequence {
     /// 6. incr = 1 allocated = 10, value = 10
     /// 7. next_value() -> 11
     fn needs_allocation(&self) -> bool {
-        self.value == self.schema.allocated
+        // In order to yield a value, it must be strictly less than the allocation amount,
+        // because on restart we will begin at the allocation amount.
+        self.value >= self.schema.allocated
     }
 
     pub(super) fn set_allocation(&mut self, allocated: i128) {

crates/core/src/db/relational_db.rs

@@ -1456,6 +1456,7 @@ mod tests {
     use commitlog::payload::txdata;
     use commitlog::Commitlog;
     use durability::EmptyHistory;
+    use pretty_assertions::assert_eq;
     use spacetimedb_data_structures::map::IntMap;
     use spacetimedb_lib::error::ResultTest;
     use spacetimedb_lib::Identity;
@@ -2197,9 +2198,17 @@ mod tests {
             fn visit_delete<'a, R: BufReader<'a>>(
                 &mut self,
                 table_id: TableId,
-                _reader: &mut R,
+                reader: &mut R,
             ) -> Result<Self::Row, Self::Error> {
-                bail!("unexpected delete for table: {table_id}")
+                // Allow specifically deletes from `st_sequence`,
+                // since the transactions in this test will allocate sequence values.
+                if table_id != ST_SEQUENCES_ID {
+                    bail!("unexpected delete for table: {table_id}")
+                }
+                let ty = self.sys.get(&table_id).unwrap();
+                let row = ProductValue::decode(ty, reader)?;
+                log::debug!("delete: {table_id} {row:?}");
+                Ok(())
             }
 
             fn skip_row<'a, R: BufReader<'a>>(

crates/core/src/vm.rs

@@ -625,6 +625,7 @@ pub(crate) mod tests {
     };
     use crate::db::relational_db::tests_utils::TestDB;
     use crate::execution_context::ExecutionContext;
+    use pretty_assertions::assert_eq;
     use spacetimedb_lib::error::ResultTest;
     use spacetimedb_sats::db::auth::{StAccess, StTableType};
     use spacetimedb_sats::db::def::{ColumnDef, IndexDef, IndexType, TableSchema};
@@ -848,7 +849,7 @@ pub(crate) mod tests {
             start: ST_RESERVED_SEQUENCE_RANGE as i128 + 1,
             min_value: 1,
             max_value: i128::MAX,
-            allocated: ST_RESERVED_SEQUENCE_RANGE as i128 * 2,
+            allocated: ST_RESERVED_SEQUENCE_RANGE as i128,
         }
         .into();
         check_catalog(&db, ST_SEQUENCES_NAME, st_sequence_row, q, schema);

crates/sats/src/db/def.rs

@@ -11,7 +11,12 @@ use spacetimedb_primitives::*;
 use std::sync::Arc;
 
 /// The default preallocation amount for sequences.
-pub const SEQUENCE_PREALLOCATION_AMOUNT: i128 = 4_096;
+///
+/// Start with no values allocated. The first time we advance the sequence,
+/// we will allocate [`SEQUENCE_ALLOCATION_STEP`] values.
+pub const SEQUENCE_PREALLOCATION_AMOUNT: i128 = 0;
+/// The amount sequences allocate each time they over-run their allocation.
+pub const SEQUENCE_ALLOCATION_STEP: i128 = 4096;
 
 impl_deserialize!([] Constraints, de => Self::try_from(de.deserialize_u8()?)
     .map_err(|_| de::Error::custom("invalid bitflags for `Constraints`"))