GNOME Bugzilla – Bug 652175
libebook: Allow a quick check for "data changed".
Last modified: 2013-09-14 16:55:33 UTC
The libebook API should offer a way to discover if any data whatsoever has changed. This would, for instance, help SyncEvolution to avoid unnecessary detailed checking. This could just check the modification dates of local storage files, such as the BDB .db file. This would be much quicker than listing items for the whole database and comparing their revisions strings, as is currently necessary. This would require small API additions, to check that the opaque revision strings are unchanged, such as gchar* e_book_get_revision(EBook *book) gboolean e_book_check_revision(EBook *book , const gchar* revision) We hope to provide a patch for this, so please let us know if it's likely to be acceptable.
Ok I'm about to try a patch for this. I have 2 ideas for the patch and not sure which one to go with but leaning against using a timestamp a.) We do use the addressbook.db file's modification timestamp, whenever the backend is requested ->get_revision() then the backend needs to explicitly *sync* the database to file. Assuming that when we call db->sync() it will not modify the file unless there are pending changes to write, then we can reliably provide an opaque revision string using the addressbook.db timestamp. The problem arises if we don't have control on the behavior of db->sync() (which is unclear to me after reading some basic man page): a.) call is made to e_book_get_revision () b.) call is made to e_book_commit_contact () c.) another call is immediately made to e_book_get_revision () In this scenario, the revision returned from step a.) and c.) must differ. If db->sync() is not called then we risk a race until the database feels like syncing... and if db->sync() *always* results in an update to the modification time of addressbook.db, then every call to e_book_get_revision() will return a new timestamp which would render the whole thing pointless. b.) Instead of using the modification time, we can use a global field in the database (I believe there is one already stored for the overall version of the DB structure), the field could be an integer revision field or also just a date stamp. Then we can do something completely independent of how BDB decides to flush to disk: o DB Revision field starts at 0 or NULL o When the addressbook starts up, it reads the current revision, stores it locally and and sets the "locally-modified" state to FALSE initially (locally-modified would just be a boolean state in the backend, not a field in the BDB). o When any contact is added/removed/modified, the "locally-modified" state turns to TRUE o When the backend->get_revision() is called: if (locally_modified) { increment_revision_and_store_in_bdb(); locally_modified = FALSE; } return current_revision; o Additionally, whenever the addressbook shuts down and possibly periodically (every hour or every day ?) then the check could be run again: if (locally_modified) { increment_revision_and_store_in_bdb(); locally_modified = FALSE; } o Asides that, there is always the case that EDS may crash for whatever reason. Possibly we would want to additionally do something like: a.) When starting the addressbook, immediately set the global "consistent-revision" field to FALSE b.) When cleanly shutting down EDS, update the current revision string if needed in the DB and set "consistent-revision" field to TRUE before shutting down. c.) When recovering from a crash, the "consistent-revision" field is FALSE at startup time... OOPS, go ahead and increment the overall revision field in case we had modifications before the crash and before incrementing the revision field. The goals of the second approach are basically: a.) Make sure that we have a consistent revision string at all times even when there are race conditions or crash recoveries. b.) Modify the revision field the absolute minimum amount of times that is necessary Currently I'm implementing the user facing apis and dbus plumbing before diving into the implementation, for now I'm starting with only: gchar *e_book_get_revision(EBook *book, GError **error); If it returns NULL, it means that the backend you are dealing with doesnt know how to handle revisions, so you would have to fall back to assuming you need to compare the vCards manually as usual.
I have a patch for this now on 'openismus-work' branch. The patch introduces both e_book_get_revision() and the async variant e_book_get_revision_async(). EGdbusBook is modified to export the new getRevision method on the dbus api, EDataBook handles the new method and treats it in the worker thread. EBookBackend takes a new optional vfunc ->get_revision(), the e_book_backend_get_revision() api itself will respond to the gdbus call with a NULL revision string in the case that a backend does not implement ->get_revision(). Finally, EBookBackendFile (the local addressbook), implements the new ->get_revision() call by adding a field to the hash table BDB: "PAS-DB-REVISION", the revision string is currently just an incremental guint64 (perhaps it should actually be a timestamp ?)... the current approach is the simplest and stablest however it might incur some overhead: a.) The revision string is loaded when the book is opened b.) Whenever a contact is added/removed/modified the revision is "bumped" and updated in the BDB (once per transaction that would cause a write the BDB, the revision string is also updated). While this leaves least possibility for any race conditions with crashing databases, it costs an extra write (however I'm uncertain whether this extra write really incurs much of a penalty, it may depend on how the BDB is setup to cache writes). The 2 related commits are as follows: commit 039d4a351e434da9c2f1b5c56b50384996947e7b Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Sun Jul 17 17:57:07 2011 -0400 Added e_book_get_revision(). e_book_get_revision() returns an opaque revision string on the entire database backend. If the backend implements ->get_revision() then every revision string returned will be guarunteed to have changed if the database has been modified in any way since the last time the revision was consulted, or similarly it will be guarunteed to have not changed if the database has not changed in any way since the last call to e_book_get_revision(). commit 3b1087e23743f10664408bf64112a5c8dcf2ca07 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Sun Jul 17 18:00:04 2011 -0400 Added test case for e_book_get_revision() Test case ensures that the revision string changes after having added and removed contacts in the book.
Ported this patch to use the new EBookClient apis and new gdbus setup from master... patches targeting eds master are in 'openismus-work-master'. In case it's helpful for review, I'm going to make sure every patch that needs review has an attachment in bugzilla... Relevant commits are: commit 44f9fdd798c0dc606b7485e8cc79360d2e3ed4dc Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jul 22 15:17:05 2011 -0400 Added test case to test e_book_client_get_revision() Added a tests/libebook/client/test-client-get-revision.c to test that the local backend supports revisions and the revisions indeed change after modifying the addressbook in any way. commit 76b68660a1d098d4df33771f7f727b3fac518f53 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jul 22 15:14:42 2011 -0400 Added e_book_client_get_revision(). Adds e_book_client_get_revision() async/sync api to access an opaque revision string which will be NULL for a backend that does not implement. Adds all the needed gdbus machinery and implements a revision in the file backend. Bug: https://bugzilla.gnome.org/show_bug.cgi?id=652175
Created attachment 192476 [details] [review] Patch to add e_book_get_revision() [targetting meego-eds branch]
Created attachment 192477 [details] [review] Patch to add e_book_client_get_revision() [targetting master]
Created attachment 192514 [details] [review] Patch to add e_book_client_get_revision() [targetting master, take 2] Found a bug in the previous patch and fixed this in 'openismus-work-master' branch, this is the updated patch. The problem was that the added "revision" hash db key was being reported along with vcards in some places (special casing needed to be added along side the same checks for the DB version db key).
Created attachment 192517 [details] [review] Patch to add e_book_get_revision() [targetting meego-eds branch, take 2] This patch is a remake of the previous e_book_get_revision() patch for meego-eds with the added bugfix backported from 'openismus-master-branch'.
Created attachment 193199 [details] [review] Patch to add e_book_client_get_revision() [targetting master, take 3] Here is a slight modification to these patches. Instead of a boring single integer for the revision string which I was thinking was kindof ugly, I made it a timestamp like the E_CONTACT_REV is created. Except I'm still appending an integer to the string since it's a very easy way to avoid race conditions where revisions might bump more than once inside the same second. New commit on openismus-work-master is: commit c450f28b692685cc8970f3bbb82587adbe51a6b1 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jul 22 19:58:20 2011 -0400 Added e_book_client_get_revision(). Adds e_book_client_get_revision() async/sync api to access an opaque revision string which will be NULL for a backend that does not implement it. Adds all the needed gdbus machinery and implements a revision in the file backend. The revision string is a timestamp + integer (the integer is incremented every revision bump to avoid any races while multiple revisions can occur inside one second). Bug: https://bugzilla.gnome.org/show_bug.cgi?id=652175
Created attachment 193200 [details] [review] Patch to add e_book_get_revision() [targetting meego-eds branch, take 3] Same change for meego-eds branch, commit on openismus-work branch is now: commit c569efe31508c8f1605bb39cc74e603d109ede2d Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Sun Jul 17 17:57:07 2011 -0400
Created attachment 198956 [details] [review] Patch to add e_book_client_get_revision() [targetting master, take 4] Refreshing patches for a huge rebase of 'openismus-work-master' off of current master branch. Attaching new and updated patch for master. New related commits for this bug on 'openismus-work-master': Test case: commit 79aa0548bd752c19a87df14da9172244f1f10f77 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jul 22 19:59:12 2011 -0400 Added test case to test e_book_client_get_revision()... Implementation: commit bf19cd6d2f425d75c524466bc415273199e435fd Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jul 22 19:58:20 2011 -0400 Added e_book_client_get_revision()....
Oh, I believe you'll not like me, but rather than introducing new API, let's have it as a backend property, which it surely is, just a backend property. It'll have some advantages as well, as you would be able to listen for change on the "property", and do your favourite steps when revision changes. I agree, it's unlikely :) Only note those are not usual GObject properties, maybe one day they will. On the first look, your new functions in e-book-backend-file.c, please name them with correct prefix, like not e_book_backend_new_revision but e_book_backend_file_new_revision . It's better when thinking where the function comes from (the local functions are also sometimes without the "e_" prefix, but that can be also suboptimal). There are more functions than this one.
Created attachment 201209 [details] [review] Add the "revision" property to the addressbook backend New patch does just about exactly the same as the old one but without the huge dbus diffs cluttering the patch ;-) (with the api names in the file backend adjusted as you requested). New commit on openismus-master-branch: commit bb1bdba64dd1aa3354af2778dff4c52174d99483 Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Thu Nov 10 21:24:34 2011 -0500 Bug #652175 Add revision property to addressbook backend This patch adds the "revision" property definition in e-book-client.h, e-book-backend.h and e-client.h as well as an implementation of the revision property in the local file backend. New commit to try the test case: commit 5de90c27b648fccafa313332cd7abc12f3332b6a Author: Tristan Van Berkom <tristan.van.berkom@gmail.com> Date: Fri Jul 22 19:59:12 2011 -0400 Added test case to test the new backend revision property Added a tests/libebook/client/test-client-get-revision.c to test that the local backend supports revisions and the revisions indeed change after modifying the addressbook.
Looks good, please commit to master with this change: - I see you added CLIENT_BACKEND_PROPERTY_REVISION - it will have its real meaning only when both clients will have it available, which means after the bug #652177. You see that e-client.h defines constants for common properties, where e-book-client.h (and e-cal-client.h) defines _only_ properties which are valid for respective EClient descendant, thus having the constant on both places (in e-client and in its descendant) doesn't make sense. Please remove it from e-book-client.h, supposing the change in ECalClient will land soon. Minor concerns: - Add a dash before the "Add" word in the commit's log first line, after Bug #. - I didn't think of this before, but in e_book_backend_file_new_revision() can be _also_ used pair g_get_current_time() and g_time_val_to_iso8601(). It can, it doesn't mean I force you :)
Review of attachment 201209 [details] [review]: Committing the patch with the change to the commit log and the removed change to e-client.h I did not change the GTimeVal thing, I'm sorry I have so little time these days... just a note though, I found myself changing my GTimeVal code for monotonic time (seems some of the glib GTimeVal functions have gone deprecated... g_source_get_time() or such...)
(In reply to comment #14) > I did not change the GTimeVal thing, I'm sorry I have so > little time these days... No problem at all, I just recalled of those functions when seeing the format you are using. > just a note though, I found myself > changing my GTimeVal code for monotonic time (seems some > of the glib GTimeVal functions have gone deprecated... g_source_get_time() or > such...) It would be surprising to see these particular two functions being gone in future releases of glib, but I agree they are doing huge API changes in glib master recently.