fsprojects / SQLProvider

A general F# SQL database erasing type provider, supporting LINQ queries, schema exploration, individuals, CRUD operations and much more besides.
https://fsprojects.github.io/SQLProvider
Other
564 stars 144 forks source link

SubmitUpdates Operation Order #758

Closed devinlyons closed 11 months ago

devinlyons commented 2 years ago

Is your feature request related to a problem? Please describe. When inserting a record with child records, there is no guarantee that the child records will be created after the parent. This causes a database error.

Describe the solution you'd like It would be good if there was some consistency in the order of the operations. Something like FIFO. Perhaps the data SubmitUpdates method could accept a ProcessingStrategy input.

Describe alternatives you've considered N/A

Additional context N/A

Thorium commented 2 years ago

What if you call multiple times submitupdates? The disadvantage may be completing transaction too early?

devinlyons commented 2 years ago

Exactly. My hope was that I could queue up the operations and commit them all in a single transaction. Otherwise, I have to manually create the transaction. That's easy enough but there's no documentation on doing that with SQLProvider. The page on CRUD operations mentions it briefly but there is no code example and I can't find anything in the API documentation.

Thorium commented 2 years ago

I went for manual transaction handling. In .NET the default TransactionScopeOption is Required meaning SQLProvider should just commit to a transaction if you have one.

This thread might help you a little bit: https://github.com/fsprojects/SQLProvider/issues/238#issuecomment-216226084

devinlyons commented 2 years ago

That's what I ended up doing. My code is working but I still think having the SubmitUpdates method perform updates in a predictable manner is a worthwhile feature.

Anyway, thanks for the help!

gbtb commented 12 months ago

Hello.

I highly agree with @devinlyons opinion, I think that having deterministic update order is really a must-have feature when working with foreign keys constraints. Looking into the code of https://github.com/fsprojects/SQLProvider/blob/b116a4bcf007b9ceb7cd07f5ce1d934a00557dd9/src/SQLProvider.Runtime/SqlRuntime.DataContext.fs#L113 It seems to me that adding Seq.sortBy (fun e -> e.Value) in the pipeline before Seq.iter should be really straightforward and it allows to use an ordering of Create() calls to ensure foreign-key friendly order of inserts.

What do you think @Thorium ?

Thorium commented 12 months ago

I'm not familiar enough of the issue, so my point of view is pretty general:

Can this be a breaking change? Can someone already trust the current order?

And what about the upsert things people are doing, (conflict strategy parameter). I have no idea of those? https://fsprojects.github.io/SQLProvider/core/crud.html#OnConflict

Maybe this sorting should be also a new additional parameter, do it or not, and by default keep the existing behaviour? But if we add that, there should be docs update, "when do you need it".

gbtb commented 12 months ago

I don't think that ConcurrentDictionary enumerator has any ordering guarantees. Well, I guess it offers some kind of ordering unlike a fully random shuffle, but trusting to that order by anybody is meaningless. And I don't think that adding sortBy is a breaking change, since we're not touching any public APIs in any way. Considering OnConflict strategy, at least in SQLite case, it supports only unique constraints or primary keys (doc)

Thorium commented 12 months ago

I'm not sure if the code change suggested will fix the issue:

The pending changes dictionary is just keeping account in memory what updates are not yet processed to database. As far as I know TryRemove is not actually caring the relationships.

The reason I'm scared about sorting is that the sorted list is not concurrent anymore, so potential thread-safety issue?

What is the sorted "Value" anyway? Is it database entity values like Key="Age", Value="22' ? If so, what is the sort by value adding?

gbtb commented 12 months ago

I'm not sure if the code change suggested will fix the issue:

The pending changes dictionary is just keeping account in memory what updates are not yet processed to database. As far as I know TryRemove is not actually caring the relationships.

The reason I'm scared about sorting is that the sorted list is not concurrent anymore, so potential thread-safety issue?

What is the sorted "Value" anyway? Is it database entity values like Key="Age", Value="22' ? If so, what is the sort by value adding?

Sorry, I misread the code and linked a wrong line :slightly_smiling_face: . Even though basic idea stands - we use already present DateTime.UTCNow (of moments when an entity was created) values of pendingChanges dictionary. I've made a draft PR to show one way of implementing this feature - https://github.com/fsprojects/SQLProvider/pull/797. It will require similar changes to all provider implementations. Another way would be to sort key-value pairs before passing them to ProcessUpdates[Async], so we can keep sorting logic in one place. However, it would require changes in method signatures of ISqlProvider.

What do you think?

Thorium commented 12 months ago

Generally looks good to me...

gbtb commented 12 months ago

Generally looks good to me...

Thanks :slightly_smiling_face: . I will submit a full PR in the next few days then.