-
Notifications
You must be signed in to change notification settings - Fork 6
Add CRDT transformation for INSERT...RETURNING #13
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Hi Janez, I'm honestly pretty skeptical about this one. It's adding a lot of code (and some of it duplicated) to support a very niche semantic. Also, I don't currently have the bandwidth to properly evaluate this functionality, or to maintain it if it goes into the code so I prefer to leave this open for now so I can pick it up if it makes sense later. In any case I would encourage you to implement it on your own package so you're not dependent on my free time. |
|
I completely understand. It is a weird one. I don't even need it personally I only went and implemented it for completeness sake. Do leave it open and if it ever speaks out to you, get back to me. |
|
Hey Daniel. Its me again. I am closing this PR, because as I have noted this is a bad solution on the face of it. I came up with a better solution, but it has quite a bit to it. TLDR: I am working on postgres support for So I kept picking at it until drift test suite passed for postgres. There is quite a bit of changes, everything is documented. All existing tests pass and I wrote additional tests for the new features/fixes. I can appreciate that you are busy and that you need time to decide whether to take these changes in - although IMO they do make libraries better without breaking anything. So I decided to give you a couple of months to make a call whether to merge these changes back in and release them as v4.0.0 and if you decline I will release them as a fork. Here is a summary of changes made and link to respective branches: sql_crdt
https://github.com/JanezStupar/sql_crdt/tree/feature/postgres_related sqlite_crdt
https://github.com/JanezStupar/sqlite_crdt/tree/feature/ver4 postgres_crdt
https://github.com/JanezStupar/postgres_crdt/tree/feature/ver4 |
|
Thanks. FYI I'm looking at the strategy used by PowerSync and there are a few concepts that I like, mainly that it run in parallel to the database monitoring for changes and performing conflict resolution on top. This has the benefit of being able to use standard tooling or non-CRDT-aware clients to make changes to the database without breaking the HLC clock, or forcing tables to have modified and is_deleted columns. I do have a few concerns about their implementation, mainly in terms of performance and that it seems to delegate server-side conflict resolution to your backend API if I'm interpreting this diagram properly: If we use a similar approach, we might greatly simplify our implementation by the fact that we would no longer need to build wrappers around every single SQL command in existence. |
|
Cool! Will definitely have a look and make up my mind first, then I will
get back to you.
For what it's worth sql_crdt has been pretty impressive for what it is. And
now that I have a full drift test suite running on it for both Postgres and
SQLite you can easily claim it is "functionally feature complete".
But I am very open to new ideas so will have a look.
Most of all - the reason I started looking for a custom solution and ended
up adopting sql_crdt is that I didn't want to lock myself (and my users) to
FAANG stuff like Firebase.
…On Tue, Oct 14, 2025 at 1:44 PM Daniel Cachapa ***@***.***> wrote:
*cachapa* left a comment (cachapa/sql_crdt#13)
<#13 (comment)>
Thanks.
FYI I'm looking at the strategy used by PowerSync
<https://www.powersync.com/> and there are a few concepts that I like,
mainly that it run in parallel to the database monitoring for changes and
performing conflict resolution on top.
This has the benefit of being able to use standard tooling or
non-CRDT-aware clients to make changes to the database without breaking the
HLC clock, or forcing tables to have modified and is_deleted columns.
I do have a few concerns about their implementation, mainly in terms of
performance and that it seems to delegate server-side conflict resolution
to your backend API if I'm interpreting this diagram properly:
image.png (view on web)
<https://github.com/user-attachments/assets/3b7577a6-a3ea-4c51-b330-dc074cef14f3>
If we use a similar approach, we might greatly simplify our implementation
by the fact that we would no longer need to build wrappers around every
single SQL command in existence.
—
Reply to this email directly, view it on GitHub
<#13 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6CFPC3ZMYYXCM2UZZXV33XTOZBAVCNFSM6AAAAACIXBJ7J2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTIMBRGM4DGOBYGQ>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***>
--
Janez Štupar,
www.janezstupar.com
|

Hi Daniel,
I have had this issue reported on
drift_crdta while ago. The issue is that sql_crdt does not supportINSERT - RETURNINGsemantic.Instead of only fixing it in
drift_crdtI thought this would be something that could improvesql_crdt.I will be frank, I don't think the solution is particularly elegant. But I guess even from standpoint of SQL it is not a particularly elegant requirement in the first place.
The reason I put the logic into the
queryfunction is that the nature of the feature is such that values have to be returned after insert. And I guess this matches the upstream API's better.Let me know if you see a better way forward.