ST sequences: respect allocated amount on restart

[gefjon]

Description of Changes

Prior to this change, a weird special case in build_sequence_state which ignored system tables' sequences
made it impossible to add new database objects (tables, indexes, &c) due to unique constraint violations in the system tables.

Changing this uncovered a cascade of other failures, as our autoinc and bootstrapping sequences had a variety of tightly-coupled counterintuitive behaviors, each papering over a previous (mis-)behavior. In particular:

• We must not replace the sequence_id autoinc column when updating st_sequence rows in MutTxId::get_next_sequence_value, because a system sequence has ID 0. This means calling insert_row_internal rather than insert.
• We should not pre-allocate sequence values during bootstrap, because doing (combined with the above patches) causes system sequences to advance one additional time, resulting in values starting from 8192 instead of 4096.
• From @kim : uniformly handle updates to either running or non-running hosts, core: Fix module update #1538 .
• From @kim : refactor to avoid mismatching program hashes and program bytes, core: Load program hash alongside bytes #1539 .

API and ABI breaking changes

N/a

Expected complexity level and risk

3 - autoinc handling is very fiddly, as evidenced by the series of intertwined bugs fixed by sequential commits in this PR.

Testing

• Published a BitCraft hotfix on the alpha staging server that added a new index, and it seems to have worked.
• Our test suite passes.

[gefjon]

Preliminary investigation of the test failure suggests:

• The sequence seq_st_table_table_id_primary_key_auto gets ID 0.
• When we attempt to allocate values from that sequence, we read its row from st_sequences and then write it back.
• Because that row has the value 0, it triggers an auto-inc replacement and gets the next value in the sequence (namely 8194; where this specific value comes from is beyond my current understanding).
• But st_column still refers to it by ID 0, so the next time we try to look it up, we get error: no such sequence!

The specific flow is:

• create_table
  • insert on st_table
    • get_next_sequence_value on sequence 0, seq_st_table_table_id_primary_key_auto
      • delete on st_sequences of the row for seq_st_table_table_id_primary_key_auto.
      • insert on st_sequences of the row for seq_st_table_table_id_primary_key_auto.
        • get_next_sequence_value on sequence 3, seq_st_sequences_sequence_id_primary_key_auto.
          • insert, delete on st_sequences of the row for seq_st_sequences_sequence_id_primary_key_auto; this keeps its nice ID of 3.
          • Return 8194 as next value for seq_st_sequences_sequence_id_primary_key_auto.
        • Replace st_sequence row for seq_st_table_id_primary_key_auto's ID with 8194.
        • insert_row_internal on st_sequences with row that has ID 8194.

And from there we're screwed.

[gefjon]

Updates:

The above comment's bug is fixed by calling insert_row_internal rather than insert in get_next_sequence_value, i.e. by skipping autoinc and unique constraint validation. These are already performed in create_sequence, and should not be repeated.

[gefjon]

After the most recent commit, I am still able to add a new index to a recently-restarted BitCraft module when publishing without -c.

[cloutiertyler]

I just left one small nit, but otherwise I'm good to approve this.

[gefjon]

Going to try my hand at writing a smoketest, then will merge.

[gefjon]

Well damn, that new smoketest is actually broken. To recreate without the Smoketest driver:

• spacetime start.
• In another window, publish a module with the following code:

  use spacetimedb::spacetimedb;
  
  #[spacetimedb(table)]
  pub struct T1 {
      id: u64,
  }
  
  #[spacetimedb(table)]
  pub struct T2 {
      id: u64,
  }
  
  #[spacetimedb(init)]
  pub fn init() {
      for id in 0..1_000 {
          T1::insert(T1 { id });
          T2::insert(T2 { id });
      }
  }

• Run spacetime subscribe test_add_index 'select T1.* from T1 join T2 on T1.id = T2.id where T2.id = 1001' --print-initial-update -n 0, see expected error.
• Restart SpacetimeDB.
• Add indexes so the module code is now:

  use spacetimedb::spacetimedb;
  
  #[spacetimedb(table)]
  #[spacetimedb(index(btree, name = "id", id))]
  pub struct T1 {
      id: u64,
  }
  
  #[spacetimedb(table)]
  #[spacetimedb(index(btree, name = "id", id))]
  pub struct T2 {
      id: u64,
  }
  
  #[spacetimedb(init)]
  pub fn init() {
      for id in 0..1_000 {
          T1::insert(T1 { id });
          T2::insert(T2 { id });
      }
  }

• Publish without -c.
• Run the above subscribe again and see the same error. It should succeed.
• Be very sad.

[gefjon]

Ok, what's really weird is that if you do:

• Publish without index.
• Republish with index.
• Republish without index.
• Restart.
• Publish with index.
• Subscribe.

Then it works.

[kim]

Pushed #1538 #1539 onto this.
For reference:

#1538:

When updating a host, that host may not currently be running, in which case it is launched. However, it may (or may not) be initialized already. In either case, it is initialized before updating.

However, the host controller handled the not-running case without considering the auto-initialization -- that is, it took the module the host was launched with to perform the update, instead of the new module.

This meant that the schema was the same, so no actual update was performed.

This patch fixes this by handling both cases uniformly, which always instantiates a module from the supplied program bytes.

It also passes the correct program hash to launch_module, so as to fix the diagnostic output.

#1539:

Passing around the pair of (hash, bytes) in the host controller proved confusionary. Also, the TxDatastore::program_bytes method would in one instance require to compute the hash unnecessarily.

Thus, refactor to always load the hash and bytes together, and introduce a named pair Program containing both.