From 72867563c332741500631670755a0eb557142b00 Mon Sep 17 00:00:00 2001 From: Matt <1009003+tantaman@users.noreply.github.com> Date: Fri, 13 Oct 2023 08:43:09 -0400 Subject: [PATCH] make site_id required 1. We compact out side ids so there isn't much advantage to making it null for local writes anymore 2. This cleans up the interface so users don't have to do weird coalescing 3. This allows us to make #386 a reality by switching tie breaks to site_ids --- README.md | 8 ++--- core/rs/bundle_static/Cargo.lock | 40 ++++++++++------------ core/rs/core/src/bootstrap.rs | 2 +- core/rs/core/src/tableinfo.rs | 14 ++++---- core/rs/integration_check/Cargo.lock | 44 +++++++++++-------------- core/src/changes-vtab.test.c | 14 ++++---- core/src/crsqlite.test.c | 13 ++++---- py/correctness/tests/test_cl_merging.py | 7 ++-- 8 files changed, 66 insertions(+), 76 deletions(-) diff --git a/README.md b/README.md index 476f6b68c..18a286969 100644 --- a/README.md +++ b/README.md @@ -68,8 +68,8 @@ The full documentation site is available [here](https://vlcn.io/docs). - A function extension (`crsql_as_crr`) to upgrade existing tables to "crrs" or "conflict free replicated relations" - `SELECT crsql_as_crr('table_name')` - A virtual table (`crsql_changes`) to ask the database for changesets or to apply changesets from another database - - `SELECT "table", "pk", "cid", "val", "col_version", "db_version", COALESCE("site_id", crsql_site_id()), cl, seq FROM crsql_changes WHERE db_version > x AND site_id IS NULL` -- to get local changes - - `SELECT "table", "pk", "cid", "val", "col_version", "db_version", COALESCE("site_id", crsql_site_id()), cl, seq FROM crsql_changes WHERE db_version > x AND site_id IS NOT some_site` -- to get all changes excluding those synced from some site + - `SELECT "table", "pk", "cid", "val", "col_version", "db_version", "site_id", cl, seq FROM crsql_changes WHERE db_version > x AND site_id IS NULL` -- to get local changes + - `SELECT "table", "pk", "cid", "val", "col_version", "db_version", "site_id", cl, seq FROM crsql_changes WHERE db_version > x AND site_id IS NOT some_site` -- to get all changes excluding those synced from some site - `INSERT INTO crsql_changes VALUES ([patches received from select on another peer])` - And `crsql_begin_alter('table_name')` & `crsql_alter_commit('table_name')` primitives to allow altering table definitions that have been upgraded to `crr`s. - Until we move forward with extending the syntax of SQLite to be CRR aware, altering CRRs looks like: @@ -104,10 +104,10 @@ insert into foo (a,b) values (1,2); insert into baz (a,b,c,d) values ('a', 'woo', 'doo', 'daa'); -- ask for a record of what has changed -select "table", "pk", "cid", "val", "col_version", "db_version", COALESCE("site_id", crsql_site_id()), "cl", "seq" from crsql_changes; +select "table", "pk", "cid", "val", "col_version", "db_version", "site_id", "cl", "seq" from crsql_changes; ┌───────┬─────────────┬─────┬───────┬─────────────┬────────────┬──────────────────────────────────────┬────┬─────┐ -│ table │ pk │ cid │ val │ col_version │ db_version │ COALESCE("site_id", crsql_site_id()) │ cl │ seq │ +│ table │ pk │ cid │ val │ col_version │ db_version │ "site_id" │ cl │ seq │ ├───────┼─────────────┼─────┼───────┼─────────────┼────────────┼──────────────────────────────────────┼────┼─────┤ │ 'foo' │ x'010901' │ 'b' │ 2 │ 1 │ 1 │ x'049c48eadf4440d7944ed9ec88b13ea5' │ 1 │ 0 │ │ 'baz' │ x'010b0161' │ 'b' │ 'woo' │ 1 │ 2 │ x'049c48eadf4440d7944ed9ec88b13ea5' │ 1 │ 0 │ diff --git a/core/rs/bundle_static/Cargo.lock b/core/rs/bundle_static/Cargo.lock index a66bd87e1..a0858632d 100644 --- a/core/rs/bundle_static/Cargo.lock +++ b/core/rs/bundle_static/Cargo.lock @@ -19,32 +19,27 @@ checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" [[package]] name = "bindgen" -version = "0.63.0" +version = "0.68.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "36d860121800b2a9a94f9b5604b332d5cffb234ce17609ea479d723dbc9d3885" +checksum = "726e4313eb6ec35d2730258ad4e15b547ee75d6afaa1361a922e78e59b7d8078" dependencies = [ - "bitflags 1.3.2", + "bitflags", "cexpr", "clang-sys", "lazy_static", "lazycell", "log", "peeking_take_while", + "prettyplease", "proc-macro2", "quote", "regex", "rustc-hash", "shlex", - "syn 1.0.109", + "syn", "which", ] -[[package]] -name = "bitflags" -version = "1.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" - [[package]] name = "bitflags" version = "2.4.0" @@ -237,7 +232,7 @@ checksum = "cfb77679af88f8b125209d354a202862602672222e7f2313fdd6dc349bad4712" dependencies = [ "proc-macro2", "quote", - "syn 2.0.38", + "syn", ] [[package]] @@ -261,6 +256,16 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19b17cddbe7ec3f8bc800887bab5e717348c95ea2ca0b1bf0837fb964dc67099" +[[package]] +name = "prettyplease" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae005bd773ab59b4725093fd7df83fd7892f7d8eafb48dbd7de6e024e4215f9d" +dependencies = [ + "proc-macro2", + "syn", +] + [[package]] name = "proc-macro2" version = "1.0.67" @@ -320,7 +325,7 @@ version = "0.38.13" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d7db8590df6dfcd144d22afd1b83b36c21a18d7cbc1dc4bb5295a8712e9eb662" dependencies = [ - "bitflags 2.4.0", + "bitflags", "errno", "libc", "linux-raw-sys", @@ -357,17 +362,6 @@ dependencies = [ "sqlite3_capi", ] -[[package]] -name = "syn" -version = "1.0.109" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" -dependencies = [ - "proc-macro2", - "quote", - "unicode-ident", -] - [[package]] name = "syn" version = "2.0.38" diff --git a/core/rs/core/src/bootstrap.rs b/core/rs/core/src/bootstrap.rs index ba76c1b82..f45ba82b3 100644 --- a/core/rs/core/src/bootstrap.rs +++ b/core/rs/core/src/bootstrap.rs @@ -209,7 +209,7 @@ pub fn create_clock_table( site_id INTEGER, seq INTEGER NOT NULL, PRIMARY KEY (key, col_name) - ) WITHOUT ROWID", + ) WITHOUT ROWID, STRICT", table_name = crate::util::escape_ident(table_name), ))?; diff --git a/core/rs/core/src/tableinfo.rs b/core/rs/core/src/tableinfo.rs index cdf6428ea..536abdaf6 100644 --- a/core/rs/core/src/tableinfo.rs +++ b/core/rs/core/src/tableinfo.rs @@ -421,12 +421,12 @@ impl TableInfo { 2, ?, ?, - NULL WHERE true + 0 WHERE true ON CONFLICT DO UPDATE SET col_version = 1 + col_version, db_version = ?, seq = ?, - site_id = crsql_site_id()", + site_id = 0", table_name = crate::util::escape_ident(&self.tbl_name), sentinel = crate::c::DELETE_SENTINEL, ); @@ -471,12 +471,12 @@ impl TableInfo { 1, ?, ?, - NULL WHERE true + 0 WHERE true ON CONFLICT DO UPDATE SET col_version = CASE col_version % 2 WHEN 0 THEN col_version + 1 ELSE col_version + 2 END, db_version = ?, seq = ?, - site_id = NULL", + site_id = 0", table_name = crate::util::escape_ident(&self.tbl_name), sentinel = crate::c::INSERT_SENTINEL, ); @@ -505,12 +505,12 @@ impl TableInfo { 1, ?, ?, - NULL WHERE true + 0 WHERE true ON CONFLICT DO UPDATE SET col_version = col_version + 1, db_version = ?, seq = ?, - site_id = NULL;", + site_id = 0;", table_name = crate::util::escape_ident(&self.tbl_name), ); let ret = db.prepare_v3(&sql, sqlite::PREPARE_PERSISTENT)?; @@ -533,7 +533,7 @@ impl TableInfo { col_version = CASE col_version % 2 WHEN 0 THEN col_version + 1 ELSE col_version + 2 END, db_version = ?, seq = ?, - site_id = NULL + site_id = 0 WHERE key = ? AND col_name = ?", table_name = crate::util::escape_ident(&self.tbl_name), ); diff --git a/core/rs/integration_check/Cargo.lock b/core/rs/integration_check/Cargo.lock index 21a02308e..f5193cbd1 100644 --- a/core/rs/integration_check/Cargo.lock +++ b/core/rs/integration_check/Cargo.lock @@ -19,32 +19,27 @@ checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa" [[package]] name = "bindgen" -version = "0.63.0" +version = "0.68.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "36d860121800b2a9a94f9b5604b332d5cffb234ce17609ea479d723dbc9d3885" +checksum = "726e4313eb6ec35d2730258ad4e15b547ee75d6afaa1361a922e78e59b7d8078" dependencies = [ - "bitflags 1.3.2", + "bitflags", "cexpr", "clang-sys", "lazy_static", "lazycell", "log", "peeking_take_while", + "prettyplease", "proc-macro2", "quote", "regex", "rustc-hash", "shlex", - "syn 1.0.109", + "syn", "which", ] -[[package]] -name = "bitflags" -version = "1.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bef38d45163c2f1dde094a7dfd33ccf595c92905c8f8f4fdc18d06fb1037718a" - [[package]] name = "bitflags" version = "2.4.0" @@ -297,7 +292,7 @@ checksum = "cfb77679af88f8b125209d354a202862602672222e7f2313fdd6dc349bad4712" dependencies = [ "proc-macro2", "quote", - "syn 2.0.33", + "syn", ] [[package]] @@ -321,6 +316,16 @@ version = "0.1.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "19b17cddbe7ec3f8bc800887bab5e717348c95ea2ca0b1bf0837fb964dc67099" +[[package]] +name = "prettyplease" +version = "0.2.15" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ae005bd773ab59b4725093fd7df83fd7892f7d8eafb48dbd7de6e024e4215f9d" +dependencies = [ + "proc-macro2", + "syn", +] + [[package]] name = "proc-macro2" version = "1.0.67" @@ -380,7 +385,7 @@ version = "0.38.13" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "d7db8590df6dfcd144d22afd1b83b36c21a18d7cbc1dc4bb5295a8712e9eb662" dependencies = [ - "bitflags 2.4.0", + "bitflags", "errno", "libc", "linux-raw-sys", @@ -416,7 +421,7 @@ checksum = "4eca7ac642d82aa35b60049a6eccb4be6be75e599bd2e9adb5f875a737654af2" dependencies = [ "proc-macro2", "quote", - "syn 2.0.33", + "syn", ] [[package]] @@ -455,17 +460,6 @@ dependencies = [ "sqlite3_capi", ] -[[package]] -name = "syn" -version = "1.0.109" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "72b64191b275b66ffe2469e8af2c1cfe3bafa67b529ead792a6d0160888b4237" -dependencies = [ - "proc-macro2", - "quote", - "unicode-ident", -] - [[package]] name = "syn" version = "2.0.33" @@ -516,7 +510,7 @@ checksum = "49922ecae66cc8a249b77e68d1d0623c1b2c514f0060c27cdc68bd62a1219d35" dependencies = [ "proc-macro2", "quote", - "syn 2.0.33", + "syn", ] [[package]] diff --git a/core/src/changes-vtab.test.c b/core/src/changes-vtab.test.c index ec3da5411..56650ba0e 100644 --- a/core/src/changes-vtab.test.c +++ b/core/src/changes-vtab.test.c @@ -83,16 +83,16 @@ static void testFilters() { printf("is null\n"); assertCount(db, "SELECT count(*) FROM crsql_changes WHERE site_id IS NULL", - 3); + 0); printf("is not null\n"); assertCount( - db, "SELECT count(*) FROM crsql_changes WHERE site_id IS NOT NULL", 0); + db, "SELECT count(*) FROM crsql_changes WHERE site_id IS NOT NULL", 3); printf("equals\n"); assertCount( db, "SELECT count(*) FROM crsql_changes WHERE site_id = crsql_site_id()", - 0); + 3); // 0 rows is actually correct ANSI sql behavior. NULLs are never equal, or not // equal, to anything in ANSI SQL. So users must use `IS NOT` to check rather @@ -105,10 +105,10 @@ static void testFilters() { 0); printf("is not\n"); - // All rows are currently null for site_id + // All rows are currently equal to site_id since all rows are currently local + // writes assertCount( - db, - "SELECT count(*) FROM crsql_changes WHERE site_id IS NOT crsql_site_id()", + db, "SELECT count(*) FROM crsql_changes WHERE site_id IS crsql_site_id()", 3); // compare on db_version _and_ site_id @@ -123,7 +123,7 @@ static void testFilters() { printf("OR condition\n"); assertCount(db, "SELECT count(*) FROM crsql_changes WHERE db_version > 2 OR " - "site_id IS NULL", + "site_id IS crsql_site_id()", 3); // compare on pks, table name, other not perfectly supported columns diff --git a/core/src/crsqlite.test.c b/core/src/crsqlite.test.c index 29b292f98..adabaad0a 100644 --- a/core/src/crsqlite.test.c +++ b/core/src/crsqlite.test.c @@ -229,15 +229,16 @@ static void teste2e() { assert(rc == SQLITE_ROW); const char *tmpSiteid = (const char *)sqlite3_column_text(pStmt3, 0); - // printf("db1sid: %s\n", db1siteid); - // printf("db2sid: %s\n", db2siteid); - // printf("db3sid: %s\n", db3siteid); - // printf("tempsid: %s\n", tmpSiteid); - assert(strcmp(tmpSiteid, "NULL") == 0); + printf("db1sid: %s\n", db1siteid); + printf("db2sid: %s\n", db2siteid); + printf("db3sid: %s\n", db3siteid); + printf("tempsid: %s\n", tmpSiteid); + // printf("tmp: %s, db3: %s", tmpSiteid, db3siteid); + assert(strcmp(tmpSiteid, db3siteid) == 0); rc = sqlite3_step(pStmt3); assert(rc == SQLITE_ROW); - assert(strcmp((const char *)sqlite3_column_text(pStmt3, 0), "NULL") == 0); + assert(strcmp((const char *)sqlite3_column_text(pStmt3, 0), db3siteid) == 0); sqlite3_finalize(pStmt3); rc = sqlite3_prepare_v2(db2, "SELECT * FROM foo ORDER BY a ASC", -1, &pStmt2, diff --git a/py/correctness/tests/test_cl_merging.py b/py/correctness/tests/test_cl_merging.py index 6fa5ee2cb..24d162a19 100644 --- a/py/correctness/tests/test_cl_merging.py +++ b/py/correctness/tests/test_cl_merging.py @@ -56,7 +56,7 @@ def sync_left_to_right_exact_version(l, r, db_version): def sync_left_to_right_include_siteid(l, r, since): changes = l.execute( - "SELECT [table], pk, cid, val, col_version, db_version, coalesce(site_id, crsql_site_id()), cl, seq FROM crsql_changes WHERE db_version > ?", (since,)) + "SELECT [table], pk, cid, val, col_version, db_version, site_id, cl, seq FROM crsql_changes WHERE db_version > ?", (since,)) for change in changes: r.execute( "INSERT INTO crsql_changes VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)", change) @@ -69,7 +69,7 @@ def sync_left_to_right_include_siteid(l, r, since): def sync_left_to_right_normal_delta_state(l, r, since): r_siteid = r.execute("SELECT crsql_site_id()").fetchone()[0] changes = l.execute( - "SELECT [table], pk, cid, val, col_version, db_version, coalesce(site_id, crsql_site_id()), cl, seq FROM crsql_changes WHERE db_version > ? AND site_id IS NOT ?", + "SELECT [table], pk, cid, val, col_version, db_version, site_id, cl, seq FROM crsql_changes WHERE db_version > ? AND site_id IS NOT ?", (since, r_siteid)) largest_version = 0 for change in changes: @@ -83,7 +83,7 @@ def sync_left_to_right_normal_delta_state(l, r, since): def sync_left_to_right_single_vrsn(l, r, vrsn): r_siteid = r.execute("SELECT crsql_site_id()").fetchone()[0] changes = l.execute( - "SELECT [table], pk, cid, val, col_version, db_version, coalesce(site_id, crsql_site_id()), cl, seq FROM crsql_changes WHERE db_version = ? AND site_id IS NOT ?", + "SELECT [table], pk, cid, val, col_version, db_version, site_id, cl, seq FROM crsql_changes WHERE db_version = ? AND site_id IS NOT ?", (vrsn, r_siteid)) for change in changes: r.execute( @@ -276,6 +276,7 @@ def test_pr_299_scenario(): # c2 should have accepted all the changes given the higher causal length # a = 1, b = 1, cl = 3 # note: why is site_id missing??? Ah, it is missing since we don't coalesce to get it. This is expected. + # TODO: should no longer be missing site_id! assert (changes == [('foo', b'\x01\t\x01', '-1', None, 3, 3, None, 3, 0), ('foo', b'\x01\t\x01', 'b', 1, 1, 3, None, 3, 1)]) # c2 and c1 should match in terms of data