diff --git a/core/rs/core/src/changes_vtab_write.rs b/core/rs/core/src/changes_vtab_write.rs index 2a3848ad7..de9b0b965 100644 --- a/core/rs/core/src/changes_vtab_write.rs +++ b/core/rs/core/src/changes_vtab_write.rs @@ -27,13 +27,11 @@ use crate::util::slab_rowid; */ fn did_cid_win( db: *mut sqlite3, - ext_data: *mut crsql_ExtData, insert_tbl: &str, tbl_info: &TableInfo, unpacked_pks: &Vec, key: sqlite::int64, insert_val: *mut sqlite::value, - insert_site_id: &[u8], col_name: &str, col_version: sqlite::int64, errmsg: *mut *mut c_char, @@ -77,19 +75,7 @@ fn did_cid_win( } // versions are equal - // need to compare site ids - let ret = unsafe { - let my_site_id = core::slice::from_raw_parts((*ext_data).siteId, 16); - insert_site_id.cmp(my_site_id) as c_int - }; - - // site id lost. - if ret <= 0 { - return Ok(false); - } - - // site id won - // last thing, compare values to ensure we're not changing state on equal values + // need to compare values let col_val_stmt_ref = tbl_info.get_col_value_stmt(db, col_name)?; let col_val_stmt = col_val_stmt_ref.as_ref().ok_or(ResultCode::ERROR)?; @@ -105,9 +91,9 @@ fn did_cid_win( let local_value = col_val_stmt.column_value(0)?; let ret = crsql_compare_sqlite_values(insert_val, local_value); reset_cached_stmt(col_val_stmt.stmt)?; - // insert site id won and values differ. We should take the update. + // value won, take value // if values are the same (ret == 0) then we return false and do not take the update - return Ok(ret != 0); + return Ok(ret > 0); } _ => { // ResultCode::DONE would happen if clock values exist but actual values are missing. @@ -600,13 +586,11 @@ unsafe fn merge_insert( || !row_exists_locally || did_cid_win( db, - (*tab).pExtData, insert_tbl, &tbl_info, &unpacked_pks, key, insert_val, - insert_site_id, insert_col, insert_col_vrsn, errmsg, diff --git a/core/src/crsqlite.test.c b/core/src/crsqlite.test.c index fe3c49df8..e27f01d7a 100644 --- a/core/src/crsqlite.test.c +++ b/core/src/crsqlite.test.c @@ -440,20 +440,6 @@ static sqlite3_int64 getDbVersion(sqlite3 *db) { return db2v; } -static void *getSiteId(sqlite3 *db) { - sqlite3_stmt *pStmt = 0; - int rc = sqlite3_prepare_v2(db, "SELECT crsql_site_id()", -1, &pStmt, 0); - if (rc != SQLITE_OK) { - return 0; - } - - sqlite3_step(pStmt); - void *site = sqlite3_column_blob(pStmt, 0); - sqlite3_finalize(pStmt); - - return site; -} - static void testLamportCondition() { printf("LamportCondition\n"); // syncing from A -> B, while no changes happen on B, moves up @@ -525,15 +511,6 @@ static void noopsDoNotMoveClocks() { rc += sqlite3_open(":memory:", &db1); rc += sqlite3_open(":memory:", &db2); - void *db1SiteId = getSiteId(db1); - void *db2SiteId = getSiteId(db2); - int cmp = memcmp(db1SiteId, db2SiteId, 16); - if (cmp > 0) { - sqlite3 *temp = db1; - db1 = db2; - db2 = temp; - } - // swap dbs based on site id compare to make it a noop. rc += sqlite3_exec( @@ -570,10 +547,6 @@ static void noopsDoNotMoveClocks() { sqlite3_int64 db1vPost = getDbVersion(db1); sqlite3_int64 db2vPost = getDbVersion(db2); - // TODO: we still need to compare values so as not to bump the db_version - // forward on a no-difference - // this fails sometimes because site id winning. - printf("db1 pre: %lld db2 post: %lld", db1vPre, db2vPost); assert(db1vPre == db2vPost); assert(db1vPre == db1vPost); diff --git a/core/src/rows-impacted.test.c b/core/src/rows-impacted.test.c index ad3e9d0d7..4e3c03df3 100644 --- a/core/src/rows-impacted.test.c +++ b/core/src/rows-impacted.test.c @@ -314,7 +314,7 @@ static void testCreateThatDoesNotChangeAnything() { printf("\t\e[0;32mSuccess\e[0m\n"); } -static void testValueWouldWinButSiteIdLoses() { +static void testValueWin() { printf("ValueWin\n"); int rc = SQLITE_OK; char *err = 0; @@ -330,33 +330,6 @@ static void testValueWouldWinButSiteIdLoses() { 0, 0, &err); sqlite3_prepare_v2(db, "SELECT crsql_rows_impacted()", -1, &pStmt, 0); sqlite3_step(pStmt); - // value is greater but site id lower, a loss and now rows changed. - assert(sqlite3_column_int(pStmt, 0) == 0); - sqlite3_finalize(pStmt); - rc += sqlite3_exec(db, "COMMIT", 0, 0, 0); - assert(rc == SQLITE_OK); - - crsql_close(db); - printf("\t\e[0;32mSuccess\e[0m\n"); -} - -static void testSiteIdWin() { - printf("SiteIdWin\n"); - int rc = SQLITE_OK; - char *err = 0; - sqlite3 *db = createDb(); - sqlite3_stmt *pStmt = 0; - - rc = sqlite3_exec(db, "INSERT INTO foo VALUES (1, 2)", 0, 0, 0); - - rc = sqlite3_exec(db, "BEGIN", 0, 0, 0); - rc += sqlite3_exec(db, - "INSERT INTO crsql_changes VALUES ('foo', X'010901', 'b', " - "3, 1, 1, X'FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFF', 1, 1)", - 0, 0, &err); - sqlite3_prepare_v2(db, "SELECT crsql_rows_impacted()", -1, &pStmt, 0); - sqlite3_step(pStmt); - // site id is larger, a win assert(sqlite3_column_int(pStmt, 0) == 1); sqlite3_finalize(pStmt); rc += sqlite3_exec(db, "COMMIT", 0, 0, 0); @@ -401,8 +374,7 @@ void rowsImpactedTestSuite() { testUpdateThatDoesNotChangeAnything(); testDeleteThatDoesNotChangeAnything(); testCreateThatDoesNotChangeAnything(); - testValueWouldWinButSiteIdLoses(); - testSiteIdWin(); + testValueWin(); testClockWin(); testDelete(); } \ No newline at end of file diff --git a/py/correctness/tests/test_cl_merging.py b/py/correctness/tests/test_cl_merging.py index 716a04c97..6ce34574d 100644 --- a/py/correctness/tests/test_cl_merging.py +++ b/py/correctness/tests/test_cl_merging.py @@ -28,10 +28,6 @@ def make_simple_schema(): return c -def get_site_id(c): - return c.execute("SELECT crsql_site_id()").fetchone()[0] - - def make_pko_schema(): c = connect(":memory:") c.execute("CREATE TABLE foo (a INTEGER PRIMARY KEY NOT NULL) STRICT;") @@ -841,19 +837,10 @@ def test_larger_col_version_same_cl(): close(c2) -# should be a no-op. -# values do not break ties. -# site id loses on the merge def test_larger_col_value_same_cl_and_col_version(): c1 = make_simple_schema() c2 = make_simple_schema() - # greater site id wins so we need to swap - if get_site_id(c1) > get_site_id(c2): - temp = c1 - c1 = c2 - c2 = temp - c1.execute("INSERT INTO foo VALUES (1, 4)") c1.commit() c2.execute("INSERT INTO foo VALUES (1, 1)") @@ -861,12 +848,6 @@ def test_larger_col_value_same_cl_and_col_version(): sync_left_to_right(c1, c2, 0) - assert (c1.execute("SELECT * FROM foo").fetchall() != - c2.execute("SELECT * FROM foo").fetchall()) - - sync_left_to_right(c2, c1, 0) - - # swapping direcitons it'll merge because the other guy had the bigger site id assert (c1.execute("SELECT * FROM foo").fetchall() == c2.execute("SELECT * FROM foo").fetchall()) diff --git a/py/correctness/tests/test_sync.py b/py/correctness/tests/test_sync.py index 4526cf6ef..c7f801ff4 100644 --- a/py/correctness/tests/test_sync.py +++ b/py/correctness/tests/test_sync.py @@ -155,30 +155,17 @@ def test_delete(): close(db) -# returns two dbs. The lower site id being the first one returned. -def create_and_swap(): - db1 = connect(":memory:") - db2 = connect(":memory:") - if get_site_id(db1) > get_site_id(db2): - temp = db1 - db1 = db2 - db2 = temp - - return (db1, db2) - -# TODO: create db _then swap_ then create rows then check original invariants -# Col? not exists case so entry created and default filled in - - def test_merging_on_defaults(): - def setup_db1_schema(db1): + def create_db1(): + db1 = connect(":memory:") db1.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b DEFAULT 0);") db1.execute("INSERT INTO foo (a) VALUES (1);") db1.execute("SELECT crsql_as_crr('foo');") db1.commit() return db1 - def setup_db2_schema(db2): + def create_db2(): + db2 = connect(":memory:") db2.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b DEFAULT 0);") db2.execute("INSERT INTO foo VALUES (1, 2);") db2.execute("SELECT crsql_as_crr('foo');") @@ -186,9 +173,8 @@ def setup_db2_schema(db2): return db2 # test merging from thing with records (db2) to thing without records for default cols (db1) - (db1, db2) = create_and_swap() - setup_db1_schema(db1) - setup_db2_schema(db2) + db1 = create_db1() + db2 = create_db2() sync_left_to_right(db2, db1, 0) # db1 has changes from db2 @@ -201,9 +187,8 @@ def setup_db2_schema(db2): close(db1) close(db2) - (db1, db2) = create_and_swap() - setup_db1_schema(db1) - setup_db2_schema(db2) + db1 = create_db1() + db2 = create_db2() sync_left_to_right(db1, db2, 0) @@ -230,19 +215,18 @@ def setup_db2_schema(db2): # this value will be less than the default def test_merging_larger_backfilled_default(): def create_dbs(): - (db2, db1) = create_and_swap() + db1 = connect(":memory:") db1.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b DEFAULT 4);") db1.execute("INSERT INTO foo (a) VALUES (1);") db1.execute("SELECT crsql_as_crr('foo');") db1.commit() + db2 = connect(":memory:") db2.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b DEFAULT 4);") db2.execute("SELECT crsql_as_crr('foo');") db2.commit() - db2.execute("INSERT INTO foo (a,b) VALUES (1,2);") db2.commit() - return (db1, db2) (db1, db2) = create_dbs() @@ -298,7 +282,8 @@ def test_db_version_moves_as_expected_post_alter(): # DB2 has a row with all columns having clock records since value was set explicityl # The default value with no records should always be overridden in all cases def test_merging_on_defaults2(): - def setup_db1_schema(db1): + def create_db1(): + db1 = connect(":memory:") db1.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b DEFAULT 4);") db1.execute("SELECT crsql_as_crr('foo');") db1.commit() @@ -314,7 +299,8 @@ def setup_db1_schema(db1): db1.commit() return db1 - def setup_db2_schema(db2): + def create_db2(): + db2 = connect(":memory:") db2.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b DEFAULT 4);") db2.execute("SELECT crsql_as_crr('foo');") db2.commit() @@ -328,9 +314,9 @@ def setup_db2_schema(db2): db2.commit() return db2 - (db1, db2) = create_and_swap() - setup_db1_schema(db1) - setup_db2_schema(db2) + # test merging from thing with records (db2) to thing without records for default cols (db1) + db1 = create_db1() + db2 = create_db2() sync_left_to_right(db2, db1, 0) @@ -338,67 +324,34 @@ def setup_db2_schema(db2): site_id1 = get_site_id(db1) site_id2 = get_site_id(db2) - assert (changes == [('foo', - b'\x01\t\x01', - 'b', - 2, - 1, - 2, - site_id2, - 1, - 0), - ('foo', - b'\x01\t\x01', - 'c', - 3, - 1, - 2, - site_id2, - 1, - 1)]) + assert (changes == [('foo', b'\x01\t\x01', 'b', 4, 1, 1, site_id1, 1, 0), + ('foo', b'\x01\t\x01', 'c', 3, 1, 2, site_id2, 1, 1)]) close(db1) close(db2) - (db1, db2) = create_and_swap() - setup_db1_schema(db1) - setup_db2_schema(db2) + db1 = create_db1() + db2 = create_db2() site_id1 = get_site_id(db1) site_id2 = get_site_id(db2) sync_left_to_right(db1, db2, 0) changes = db2.execute("SELECT * FROM crsql_changes").fetchall() - assert (changes == [('foo', - b'\x01\t\x01', - 'b', - 2, - 1, - 1, - site_id2, - 1, - 0), - ('foo', - b'\x01\t\x01', - 'c', - 3, - 1, - 1, - site_id2, - 1, - 1)]) + assert (changes == [ # db2 c 3 wins given columns with no value after an alter + # do no merging + ('foo', b'\x01\t\x01', 'c', 3, 1, 1, site_id2, 1, 1), + # Move db version since b lost on db2. + # b had the value 2 on db2. + ('foo', b'\x01\t\x01', 'b', 4, 1, 2, site_id1, 1, 0)]) def create_basic_db(): db = connect(":memory:") - setup_basic_db(db) - return db - - -def setup_basic_db(db): db.execute("CREATE TABLE foo (a PRIMARY KEY NOT NULL, b);") db.execute("SELECT crsql_as_crr('foo');") db.commit() + return db def test_merge_same(): @@ -428,11 +381,10 @@ def make_dbs(): assert (changes == [('foo', b'\x01\t\x01', 'b', 2, 1, 1, site_id, 1, 0)]) -def test_merge_matching_clocks_lesser_siteid(): +def test_merge_matching_clocks_lesser_value(): def make_dbs(): - (db1, db2) = create_and_swap() - setup_basic_db(db1) - setup_basic_db(db2) + db1 = create_basic_db() + db2 = create_basic_db() db1.execute("INSERT INTO foo (a,b) VALUES (1,1);") db1.commit()