After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 730950 - Transaction support
Transaction support
Status: RESOLVED FIXED
Product: gom
Classification: Other
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: Gom Maintainers
Gom Maintainers
Depends on:
Blocks:
 
 
Reported: 2014-05-29 18:01 UTC by Mathieu Bridon
Modified: 2014-11-05 11:54 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
gom: Export gom_resource_do_save() internally (1.44 KB, patch)
2014-09-21 17:30 UTC, Bastien Nocera
none Details | Review
gom: Add support for batches inserts (8.61 KB, patch)
2014-09-21 17:30 UTC, Bastien Nocera
none Details | Review
tests: Add test for batched writes (4.04 KB, patch)
2014-09-21 17:30 UTC, Bastien Nocera
none Details | Review
gom: Add support for batched writes (10.62 KB, patch)
2014-09-21 19:45 UTC, Bastien Nocera
reviewed Details | Review
gom: Add support for batched writes (11.97 KB, patch)
2014-09-21 20:48 UTC, Bastien Nocera
none Details | Review
tests: Add test for batched writes (4.02 KB, patch)
2014-09-21 20:49 UTC, Bastien Nocera
none Details | Review
gom: Add support for batched writes (12.23 KB, patch)
2014-09-21 21:12 UTC, Bastien Nocera
none Details | Review

Description Mathieu Bridon 2014-05-29 18:01:36 UTC
At the moment, when I try to INSERT lots of entries in my SQLite DB with gom, things take a long time.

This is because each request is run in its own transaction.

It would be great if gom had an API to allow adding/updating/deleting lots of items at once.

I'd even be willing to give a try at implementing it, as this is pretty much a blocker for me. :)

Here's an idea for what the API could look like.

    GomAdapter *adapter;
    ItemResource *resource1, *resource2;
    
    // Create a new transaction
    GomTransaction *transaction = gom_transaction_new();
    
    // Add some resources to the transaction
    resource1 = g_object_new(...);
    transaction_add_resource(transaction, resource1, &error);
    
    resource2 = g_object_new(...);
    transaction_add_resource(transaction, resource2, &error);

    // If everything went well
    transaction_commit(transaction, &error);
    
    // If something went wrong
    transaction_rollback(transaction, &error);

What do you think?
Comment 1 Christian Hergert 2014-05-29 18:13:19 UTC
You indeed have found the Tao of SQLite :)

Transactions and difficult to get "right" (for some definition of right), so until we could get there, I've used https://git.gnome.org/browse/gom/tree/gom/gom-adapter.h#n78 (gom_adapter_queue_write) which will give you a callback in the SQLite thread. You can do everything synchronous from there, including BEGIN/COMMIT. But yes, this is pretty much a hack at the moment.

We really need to think through the transaction semantics since someday we'd like to support more than SQLite. My instinct tells me we should have an interface that is shared between the repository and the transaction itself. This would allow us to potentially set the "repository" field on an object (or better name perhaps), and still use the gom_resource_* async functions.

Thoughts?
Comment 2 Mathieu Bridon 2014-05-29 18:37:12 UTC
> (gom_adapter_queue_write) which will give you a callback in the SQLite thread.
> You can do everything synchronous from there, including BEGIN/COMMIT.

I actually tried using it, but didn't really succeed.

> My instinct tells me we should have an
> interface that is shared between the repository and the transaction itself.
> This would allow us to potentially set the "repository" field on an object (or
> better name perhaps), and still use the gom_resource_* async functions.

Yeah, with the proposal I made, it's not clear what happens when there is an existing transaction and at the same time the application calls gom_resource_save_sync (for example)

I'm also not entirely sure what should happen in a multi-threaded app: each thread has its own transaction? Can multiple threads access the same transaction? Etc...
Comment 3 Christian Hergert 2014-05-29 21:16:13 UTC
(In reply to comment #2)
> Yeah, with the proposal I made, it's not clear what happens when there is an
> existing transaction and at the same time the application calls
> gom_resource_save_sync (for example)

Yeah, we'll need a planning session to figure this stuff out.

> I'm also not entirely sure what should happen in a multi-threaded app: each
> thread has its own transaction? Can multiple threads access the same
> transaction? Etc...

I really don't think you want a transaction to live between multiple threads. If we were writing a commercial database with MVCC, we might need that. Certainly not for general purpose desktop/mobile/phone applications.
Comment 4 Bastien Nocera 2014-08-06 09:31:39 UTC
Discussing this at GUADEC, we thought that we should add something similar to GomResourceGroup, but for writes (as opposed to read queries). Then one would call something like gom_writes_group_save() and it would do all this within a single transaction.

Finding a name for this "write" object is probably the hardest part of implementing this though ;)
Comment 5 Bastien Nocera 2014-09-21 17:30:46 UTC
Created attachment 286744 [details] [review]
gom: Export gom_resource_do_save() internally

So it can be used for saving when already in worker thread.
Comment 6 Bastien Nocera 2014-09-21 17:30:51 UTC
Created attachment 286745 [details] [review]
gom: Add support for batches inserts

XXX: Missing async version
Comment 7 Bastien Nocera 2014-09-21 17:30:56 UTC
Created attachment 286746 [details] [review]
tests: Add test for batched writes
Comment 8 Bastien Nocera 2014-09-21 19:45:43 UTC
Created attachment 286754 [details] [review]
gom: Add support for batched writes
Comment 9 Christian Hergert 2014-09-21 20:22:38 UTC
Review of attachment 286754 [details] [review]:

Pretty sure we want to allow various types of subclasses to co-exist within the resource-group, but other than that, LGTM.

::: gom/gom-resource-group.c
@@ -58,0 +64,11 @@
+GomResourceGroup *
+gom_resource_group_new (GomRepository *repository,
+                        GType          resource_type)
... 8 more ...

Part of me wishes this was an "is-writable" construct param, so that _constructed() could know about it in the future. Not sure it's worth the effort now though.

@@ -58,0 +64,21 @@
+GomResourceGroup *
+gom_resource_group_new (GomRepository *repository,
+                        GType          resource_type)
... 18 more ...

g_type_is_a() so that this can work with sub-classes?

@@ -58,0 +64,67 @@
+GomResourceGroup *
+gom_resource_group_new (GomRepository *repository,
+                        GType          resource_type)
... 64 more ...

This was a bit deceiving at first, but looks right. Iterate through all items, freeing each as we iterate to avoid a second _list_foreach() to unref, but only execute do_save() until we've failed.

Although, we do a list_free() at the end anyway, wish there was a g_list_free_with_destroy().
Comment 10 Bastien Nocera 2014-09-21 20:48:07 UTC
(In reply to comment #9)
> Review of attachment 286754 [details] [review]:
> 
> Pretty sure we want to allow various types of subclasses to co-exist within the
> resource-group, but other than that, LGTM.

Right, good idea.

> ::: gom/gom-resource-group.c
> @@ -58,0 +64,11 @@
> +GomResourceGroup *
> +gom_resource_group_new (GomRepository *repository,
> +                        GType          resource_type)
> ... 8 more ...
> 
> Part of me wishes this was an "is-writable" construct param, so that
> _constructed() could know about it in the future. Not sure it's worth the
> effort now though.

Done.

> @@ -58,0 +64,21 @@
> +GomResourceGroup *
> +gom_resource_group_new (GomRepository *repository,
> +                        GType          resource_type)
> ... 18 more ...
> 
> g_type_is_a() so that this can work with sub-classes?

I've removed that.

> @@ -58,0 +64,67 @@
> +GomResourceGroup *
> +gom_resource_group_new (GomRepository *repository,
> +                        GType          resource_type)
> ... 64 more ...
> 
> This was a bit deceiving at first, but looks right.

This is to avoid half the list having been processed if we encounter an error, so half the items have been freed after the loop.

> Iterate through all items,
> freeing each as we iterate to avoid a second _list_foreach() to unref, but only
> execute do_save() until we've failed.
> 
> Although, we do a list_free() at the end anyway, wish there was a
> g_list_free_with_destroy().

It would probably look nicer with a GptrArray. What do you think?
Comment 11 Bastien Nocera 2014-09-21 20:48:43 UTC
Created attachment 286756 [details] [review]
gom: Add support for batched writes
Comment 12 Bastien Nocera 2014-09-21 20:49:06 UTC
Created attachment 286757 [details] [review]
tests: Add test for batched writes
Comment 13 Christian Hergert 2014-09-21 20:54:09 UTC
(In reply to comment #10)
> It would probably look nicer with a GptrArray. What do you think?

Yeah let's do that. Especially since it only ever prepend/reverse the list.
Comment 14 Bastien Nocera 2014-09-21 21:12:55 UTC
Created attachment 286759 [details] [review]
gom: Add support for batched writes
Comment 15 Bastien Nocera 2014-11-04 19:00:14 UTC
This has been pushed, but I think that the performance gains aren't that interested when compared to before we landed the very necessary patches from bug 728301.

I'll close this as fixed, and we can investigate how long specific parts of the code take.
Comment 16 Bastien Nocera 2014-11-04 19:12:56 UTC
Yeah, pretty useless actually:
/GomRepository/stress: ** Message: writing items sync took 5.998153
OK
/GomRepository/stress2: ** Message: preparing batch took 1.872136
** Message: writing batch took 4.485591
OK
Comment 17 Bastien Nocera 2014-11-05 11:54:08 UTC
When using real files instead of the memory DBs:
/GomRepository/stress: ** Message: writing items sync took 13.392836
OK
/GomRepository/stress2: ** Message: preparing batch took 1.857372
** Message: writing batch took 4.632101
OK

So it seems to work fine.