Refactoring bootstrapping and adding full support for constraints & multi-column indexes #267

mamcx · 2023-09-06T18:57:23Z

Description of Changes

NOT REVIEW IT YET!

Note: This is a multi-step set of PRs to bring constraints + multi-column indexes

Step:

API and ABI

This is a breaking change to the module ABI
This is a breaking change to the module API
This is a breaking change to the ClientAPI
This is a breaking change to the SDK API

If the API is breaking, please state below what will break

cloutiertyler

I've read through most of this PR. If the bootstrapping works, it definitely looks more cleaned up and less fragile than before so that is great. I also think create_table is cleaner as well.

The one major piece of feedback is that I think we should get rid of ColumnIndexAttributes in this PR. It doesn't make any sense.

The situation should be:

table constraints:
- unique (multi-column)
- primary_key (multi-column, implies unique)

table indexes:
- btree (multi-column)
- hash (multi-column, not in this PR)

column attributes:
- auto_inc

cloutiertyler · 2023-09-09T00:04:35Z

crates/cli/Cargo.toml

@@ -17,6 +17,7 @@ bench = false
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

 [dependencies]
+spacetimedb-core = { path = "../core", version = "0.6.0" }


Why do we need core?

For access decode_token. This is unrelated to this PR but could be refactored later?

cloutiertyler · 2023-09-09T01:10:10Z

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

+        ColumnIndexAttribute::PRIMARY_KEY_AUTO,
+    )])
+    .with_indexes(&[IndexDef::for_sys_column(ST_TABLES_NAME, StTableFields::TableName, true)])
+    .into_schema(ST_TABLES_ID.0)


Ooh this is fancy! Makes sense.

cloutiertyler · 2023-09-09T01:14:11Z

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

+    .with_constraints(&[ConstraintDef::for_sys_column(
+        ST_TABLES_NAME,
+        StTableFields::TableId,
+        ColumnIndexAttribute::PRIMARY_KEY_AUTO,


I really think this should be two constraints:

UNIQUE
AUTO_INC (not really even a constraint)

I would even make the change to deprecate ColumnIndexAttribute in this PR. The bitflag thing is not the direction we want to go if we have the ability to add multiple constraints. I'm okay with doing that in a future PR as long as we leave a comment in this PR that we intend to make the change.

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

cloutiertyler · 2023-09-09T01:33:44Z

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

-            // },
-            pos: value.col_id as usize,
+impl ColumnSchema {
+    pub fn from_def(table_id: u32, col_pos: u32, column: ColumnDef) -> Self {


Why not use the From trait? This is more explicit I guess?

Yes, also to use From in callers it needs to use a tuple so I think this looks cleaner.

crates/lib/src/table.rs

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

kim

Requesting changes mainly because I think this breaks the update_database logic.

crates/bindings/src/rt.rs

kim · 2023-10-11T05:50:52Z

crates/cli/src/subcommands/generate/csharp.rs

+    for idx in unique_it {
+        let is_unique = idx.is_unique;
+
+        //Skip multi_column  indexes


Why?

I think this needs more commentary. The the multi-column indexes could be .filtered out from the iterator even.

I don't think the csharp code can deal with it. I will wait to confirm if I can remove this restriction...

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

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

crates/core/src/host/wasm_common/module_host_actor.rs

crates/core/src/host/host_controller.rs

kim · 2023-10-11T08:58:03Z

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

+    fn scan_st_tables<'a>(&'a self, tx: &'a Self::TxId) -> Result<Vec<StTableRow<String>>>;
+
+    fn scan_st_columns<'a>(&'a self, tx: &'a Self::TxId) -> Result<Vec<StColumnRow<String>>>;
+
+    fn scan_st_constraints<'a>(&'a self, tx: &'a Self::TxId) -> Result<Vec<StConstraintRow<String>>>;
+
+    fn scan_st_sequences<'a>(&'a self, tx: &'a Self::TxId) -> Result<Vec<StSequenceRow<String>>>;
+
+    fn scan_st_indexes<'a>(&'a self, tx: &'a Self::TxId) -> Result<Vec<StIndexRow<String>>>;


I find those a little unfortunate:

Do we really need this as public API?

Why scan_* when Vec is returned?

How will we decide which system tables can be collected, and ensure that new ones get a method here? Could we not pair st table ids with row types using associated types, and have a single method which works for all system tables?

Done, I created a utility struct instead.

kim · 2023-10-11T18:19:58Z

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

-        // state should be initialized eagerly here?
+        // We don't add the row here but with `MutProgrammable::set_program_hash`, but we need to register the table
+        // in the internal state.
+        // TODO: This should be moved here?


You mean set_program_hash should be moved here? That would only be possible if we could bootstrap in a transaction -- otherwise, if the init reducer fails, the hash may point to nothing.

Understood, I remove the TODO then.

crates/core/src/host/wasm_common/module_host_actor.rs

update_database should now work correctly

Centril · 2023-10-12T07:37:54Z

crates/vm/src/program.rs

@@ -2,7 +2,7 @@
 //!
 //! It carries an [EnvDb] with the functions, idents, types.
 use spacetimedb_lib::identity::AuthCtx;
-use spacetimedb_lib::relation::{MemTable, RelIter, Relation, Table};
+use spacetimedb_sats::relation::{MemTable, RelIter, Relation, Table};


It feels rather strange that MemTable should be part of SATS.

Centril · 2023-10-12T07:38:55Z

crates/vm/src/dsl.rs

+pub fn db_table(head: ProductType, name: &str, table_id: TableId) -> DbTable {
+    db_table_raw(head, name, table_id, StTableType::User, StAccess::for_name(name))
 }


Revert this please; seems like the rebase went somewhat wrong. We should not be passing &str only to clone it later on. It's better to pass owned stuff directly.

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

mamcx requested a review from cloutiertyler September 6, 2023 18:57

mamcx mentioned this pull request Sep 8, 2023

Adding multi-column indexes to the DB #243

Merged

9 tasks

cloutiertyler changed the base branch from master to mamcx/multi-column-index September 9, 2023 00:03

cloutiertyler changed the base branch from mamcx/multi-column-index to master September 9, 2023 00:04

cloutiertyler reviewed Sep 9, 2023

View reviewed changes

mamcx force-pushed the mamcx/multi-column-schema branch 2 times, most recently from 9b52f7e to 25a638c Compare September 14, 2023 18:11

mamcx mentioned this pull request Sep 15, 2023

Store the current module hash in a system table #290

Merged

5 tasks

mamcx force-pushed the mamcx/multi-column-schema branch 2 times, most recently from e205895 to eb018db Compare September 25, 2023 19:51

mamcx marked this pull request as ready for review September 25, 2023 20:30

kim mentioned this pull request Sep 29, 2023

Fix bootstrapping of ST_MODULE #342

Merged

4 tasks

cloutiertyler added the invalid label Oct 3, 2023

mamcx force-pushed the mamcx/multi-column-schema branch from d66632a to 29b517c Compare October 10, 2023 22:02

mamcx removed the invalid label Oct 10, 2023

mamcx force-pushed the mamcx/multi-column-schema branch from 9e7d30e to e5a2de7 Compare October 10, 2023 23:11

kim mentioned this pull request Oct 11, 2023

Create multi-column indexes defined in modules #386

Closed

4 tasks

kim previously requested changes Oct 11, 2023

View reviewed changes

kim reviewed Oct 11, 2023

View reviewed changes

kim self-requested a review October 12, 2023 05:45

Centril reviewed Oct 12, 2023

View reviewed changes

Centril requested changes Oct 12, 2023

View reviewed changes

kim mentioned this pull request Oct 12, 2023

Simplify/deabstract module_host_actor and wasmer_module #417

Merged

4 tasks

mamcx force-pushed the mamcx/multi-column-schema branch 2 times, most recently from bf9e4c2 to 9328408 Compare October 13, 2023 19:39

mamcx mentioned this pull request Oct 17, 2023

Refactoring IDs for system objects #439

Merged

8 tasks

mamcx mentioned this pull request Oct 26, 2023

Moving object defs to SATS #460

Merged

4 tasks

mamcx added the release-0.8 label Oct 30, 2023

mamcx mentioned this pull request Nov 8, 2023

Add integration test for the query macro #538

Merged

4 tasks

mamcx self-assigned this Nov 8, 2023

mamcx added the abi-break A PR that makes an ABI breaking change label Nov 8, 2023

mamcx marked this pull request as draft November 17, 2023 23:23

mamcx force-pushed the mamcx/multi-column-schema branch from 14938f0 to 19c2847 Compare November 17, 2023 23:25

mamcx closed this Nov 22, 2023

mamcx force-pushed the mamcx/multi-column-schema branch from 2fc996c to 419cfca Compare November 22, 2023 17:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring bootstrapping and adding full support for constraints & multi-column indexes #267

Refactoring bootstrapping and adding full support for constraints & multi-column indexes #267

mamcx commented Sep 6, 2023 •

edited

Loading

cloutiertyler left a comment

cloutiertyler Sep 9, 2023

mamcx Sep 12, 2023

cloutiertyler Sep 9, 2023

cloutiertyler Sep 9, 2023

cloutiertyler Sep 9, 2023

mamcx Sep 12, 2023

kim left a comment

kim Oct 11, 2023

mamcx Oct 11, 2023 •

edited

Loading

kim Oct 11, 2023

mamcx Oct 13, 2023

kim Oct 11, 2023

mamcx Oct 13, 2023

Centril Oct 12, 2023

Centril Oct 12, 2023

Refactoring bootstrapping and adding full support for constraints & multi-column indexes #267

Refactoring bootstrapping and adding full support for constraints & multi-column indexes #267

Conversation

mamcx commented Sep 6, 2023 • edited Loading

Description of Changes

NOT REVIEW IT YET!

API and ABI

cloutiertyler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamcx Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamcx commented Sep 6, 2023 •

edited

Loading

mamcx Oct 11, 2023 •

edited

Loading