Old DMS issues - now different?

[gentledepp]

@Mimetis as you are so close to a release, I just wanted to check off some issues that we came across in the old days of DMS.
Of course, I try to figure out whether they have changed in your code base first.
But since they bought us days and weeks of frustration and 2nd level support, I just want to make sure, they are fixed for good! :-)

1. after loading changes from the database, GetDMRowState was called for every row to determine what kind of change this would be for the client.

For example: Even if the row was already inserted and updated (it had an update timestamp), the row was sent to the client as "insert" in case it last synchronized, when that row did not yet exist.

While this may have worked with typical non-filtered sync scenarios, we had some serious issues with it.
This is, because we added some complex filtering logic to the stored procedures, so that the same row, that existed for ages, was not visible to a client until we gave it the necessary permissions.
While the stored procedure always returned the correct list of changes, GetDmRowState got confused often times. In the worst case, it would just not be able to determine a valid rowstate. In this case, this row would be ignored and not evet sent to the client.
Clearly, this caused a lot of issues.

We fixed this doing two things:

• completely removing GetDmRowState, so that a row is transferred
  • as UPDATE if it has a update_timestamp
  • as DELETED if it was marked tombstoned
  • as INSERTED otherwise
    ... thus not using the clients timestamp to infer the rowstate.
• for this to work on the receiving end of those changes, we had to switch from separate stored procedures doing INSERT and UPDATE to one UPSERT one. This is, because the row did only say for itself whether it is an INSERT,UPDATE,DELETE. This state, however, does not indicate whether the row already exists in the client database anymore. (Which is what GetDmRowState tried to guess) So even if a row is sent as UPDATE, it may not yet exist in the client database.

2. always use UPSERT instead of distinguishing between INSERT and UPDATE

This is basically what we implemented to overcome the issue of (1).
When going through the latest code base, I see that the framework does not have separate procedures for INSERT and UPDATE anymore, but you always do an UPSERT.

What is the reasoning behind it? Did you come to the same conclusion as we did?

3. sqliteprovider should only send local changes

We use the SQLite for our client databases. As our clients created weird sync conflicts, we had to dig through the code to find, that the SQLiteProviders selectchanges query mimics the server side ones that return all changes, that

• were made locally
• or by some other scopeid

The latter caused some issues, because this would mean that a client A could, in case its timestamp got messed up, also send changes that it originally received from a client B. This, in turn, would cause sync conflicts on the server side - including potential data loss.
Rememver: The timestamp on SQLite is just the system time which can easily be incorrect! (we had cases, where a row was inserted at 13:00 but updated the same day at 08:00! - completely nuts!)

Since DMS does not support P2P syncs, we removed the second clause (or by some other scopeid) so that clients only ever send their own changes.
Note: This could also be configured using a flag that indicates whether the SQLiteProvider is used in Client- or Server-Mode?

SELECT 	[side].[ProductCategoryID], 
	[base].[ParentProductCategoryID], 
	[base].[Name], 
	[base].[rowguid], 
	[base].[ModifiedDate], 
	[base].[Attribute With Space], 
	[side].[sync_row_is_tombstone], 
	[side].[update_scope_id] 
FROM [ProductCategory_tracking] [side]
LEFT JOIN [ProductCategory] [base]
ON [base].[ProductCategoryID] = [side].[ProductCategoryID]
WHERE (
	[side].[timestamp] > @sync_min_timestamp
	AND 
-        (
-                -- THIS IS BAAAD, as it may send changes of another client if the local system time is off target
-              [side].[update_scope_id] <> @sync_scope_id OR
+              Only EVER send local changes!!
               [side].[update_scope_id] IS NULL
-        )
)

4. Add a foreign-key friendly way of applying changes

DMS used to appliy changes the following way:

1. Apply deletes for each table
2. Apply inserts for each table
3. Apply updates for each table

foreign key conflict on insert/update

Now this could cause a foreign key conflict.
Lets assume, that we have a table Products that you can create Orders for. There is a foreign key Orders.ProductId.

• the client adds a new Product and also updates it. (It will be sent as UPDATE)
• the client also adds an order for the new product (so the order will be sent as INSERT)

Now, previously this would have failed, as the old DMS

• tried to insert the order which failed, because the product was not inserted yet. This is because it was send as an UPDATE and updates are applied after inserts.

Apparently, this case cannot happen any longer, as DMS changed the apply algorithm:

1. Apply deletes for each table
2. Apply inserts and updates for each table

However, you can still have a FK conflict with this!

foreign key conflict on update/delete

Lets take the same example as before: One Product and one Order for the product.

• the client changes the order to point to another Product
• thereafter, the client deletes the Product

When the client now sends the changes, DMS will (according to the new 2-step algorithm)

1. apply the delete of Product which will fail, because the Order on the server side still references it, as it has not yet been applied!

Note, that we cannot solve this by changing the order of the two tables in the sync config either!

There is, however, a very simple solution: Changing the ApplyChanges algorithm to

Foreach t in tables
     - apply inserts(t)
     - apply updates(t)

For each t tables.Reverse()
     - apply deletes

So instead of applying all deletes/updates/inserts for each table separately,
We iterate over all tables and apply inserts and updates first.

Then, we iterate over all the tables in reverse order and apply the deletes. This is to not violate any foreign key constraints when deleting rows from multiple tables that reference each other!

As you can see, this is a very, very simple but effective code-change and it works like a breeze ever since in our production environment!

[Mimetis]

Hey @gentledepp,
First of all, thanks for this detailed feedback. You know how I appreciate them !
I will make a thread post for each question (except 1 & 2 that are related)
Let's go :

1. GetDMRowState & 2. Upsert

TL;DR : You're right, and I came to the same conclusion as you. I think I've implemented the Upsert mechanism in v0.4.0

Detailed explanations

Today, there is no distinction between an Insert or Update row state. We just have a Modified state (DataRowState.Modified to be more precise, the DmSet has been replaced by the .Net Core classic DataSet implemented in .Net Core 2)

The Update query uses a Merge statement to update or insert if the row does not exists.

Here is an extract of the Sql Server statement of, for example, the Product_Update stored procedure:

;WITH [changes] AS (
	SELECT [p].[ProductID], [p].[Name], [side].[update_scope_id], [side].[timestamp], [side].[sync_row_is_tombstone]
	FROM (SELECT @ProductID as [ProductID], @Name as [Name]) AS [p]
	LEFT JOIN [Product_tracking] [side] ON 	[p].[ProductID] = [side].[ProductID]
	)
MERGE [Product] AS [base]
USING [changes] on [changes].[ProductID] = [base].[ProductID]
WHEN MATCHED AND ([changes].[timestamp] <= @sync_min_timestamp OR [changes].[timestamp] IS NULL OR [changes].[update_scope_id] = @sync_scope_id OR @sync_force_write = 1) THEN

	UPDATE SET [Name] = [changes].[Name]

WHEN NOT MATCHED BY TARGET AND ([changes].[timestamp] <= @sync_min_timestamp OR [changes].[timestamp] IS NULL OR @sync_force_write = 1) THEN

	INSERT ([ProductID], [Name])	VALUES ([changes].[ProductID], [changes].[Name])

OUTPUT  INSERTED.[ProductID]
INTO @dms_changed; -- populates the temp table with successful PKs

And here is an extract of the Update command for a Sqlite database

INSERT OR REPLACE INTO [ProductCategory]
([ProductID], [Name])
SELECT [c].[ProductID], [c].[Name] FROM (SELECT @ProductID as [ProductID], @Name as [Name) as [c]
LEFT JOIN [Product_tracking] AS [side] ON [side].[ProductID] = @ProductID
LEFT JOIN [Product] AS [base] ON [c].[ProductID] = [base].[ProductID]
WHERE ([base].[ProductID] = @ProductID AND ([side].[timestamp] < @sync_min_timestamp OR [side].[update_scope_id] = @sync_scope_id)) 
OR ([base].[ProductID] IS NULL  AND ([side].[timestamp] < @sync_min_timestamp OR [side].[timestamp] IS NULL)) 
OR @sync_force_write = 1;

[gentledepp]

Well, I am not so happy with INSERT OR REPLACE in SQLite, as this means, that we cannot have any additional rows in the sqlite database that are not synchronized.

Isn't there an INSERT OR UPDATE variant as well?

It could, for example, be necessary, to have an additonal column in the client database, that does not exist on the server, nor is it synchronized.
When you now run INSERT OR REPLACE and the row already exists localls, REPLACE would just overwrite the existing row like DELETE AND INSERT)

Thus, our local column would be cleared!

[Mimetis]

There is a new syntax in Sqlite allowing an UPSERT mode, using a macro INSERT INTO T VALUES() ON CONFLICT() DO UPDATE : https://www.sqlite.org/lang_UPSERT.html

To be honest, I think I've already tried this technic, but in my memories, I had some problem with the different tables joins and where clauses.
But once again, it's a bit old, and I just don't remember exactly what happened...
Not sure it can be applied in our scenario, but we can try (again? ) I guess :)

[gentledepp]

That would be awesome !

[Mimetis]

3. Sending Changes, from Sqlite. Mitigation on update_scope_id

I get your point on the [side].[update_scope_id] <> @sync_scope_id
Basically this condition is mostly used on the server side.
If I want to rephrase it, It basically says to the server (from a client perspective) :

Please don't send me back the changes I just sent to you, because we know that they are already in sync, since ... I just sent these changes to you server, and you applied them correctly :)

Indeed, from a client perspective, this condition is never matched, because all the rows that have different scope id than 0000-000 (server) or Null (local client) are coming from others scope and should not be sent back to the server, for any reason.

Since the SyncSqliteProvider is marked as CanBeServerProvider => false;, we know that Sqlite will never be used as a Server Database.
So far, I guess we can remove this condition in the Select_Changes command.

The only concern I have is that this problem (timestamp got messed up on the client side) is applying to MySql as well, since the timestamp is generated based on the datetime clock as well...

I will try to remove the sql condition on Sqlite and see if all tests continue to pass.
If there is no problem or regression, it will be available in v0.6.2

Will keep you updated.

[gentledepp]

The tests will pass - believe me ;-)

All tests will pass with

4. Add a foreign-key friendly way of applying changes
   as well ;-)

[Mimetis]

4. Foreign-key friendly way of applying changes

Today DMS is using this order:

For each t tables.Reverse()
     - apply deletes(t)

Foreach t in tables
     - apply upserts(t)

Changing this order, as you propose, can fail at some point as well.
The problem is that ... I don't find (yet !) an example where this new order will fail...
Damn you ! :)

Anyway, to mitigate the foreign keys constraints problematic, today DMS disable all the constraints on the tables to sync, before applying changes.
Basically, DMS does:

foreach t in tables
     - disable constraints(t)

foreach t tables.Reverse()
     - apply deletes(t)

foreach t in tables
     - apply upserts(t)

foreach t in tables
     - enable constraints(t)

Disabling / Enabling constraints can be disable with the options SyncOptions.DisableConstraintsOnApplyChanges that is set to true by default.

That being said, disabling and enabling constraints can affect performances, since it locks the entire table (see #430 (comment))

If I don't find any example that can causes a fail sync with your order, I guess it's worth a try in the next version

[gentledepp]

Well, we do have a fairly complex sync scheme and it works perfectly.
Please think it through and I believe you will come to the same conclusion as we did: That this is the most frictionless way of applying changes as it does even work if you do not disable constraints! 🚀✨😍