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 652175 - libebook: Allow a quick check for "data changed".
libebook: Allow a quick check for "data changed".
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Contacts
3.4.x (obsolete)
Other Linux
: High normal
: ---
Assigned To: evolution-addressbook-maintainers
Evolution QA team
meego
Depends on:
Blocks:
 
 
Reported: 2011-06-09 10:05 UTC by Murray Cumming
Modified: 2013-09-14 16:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Patch to add e_book_get_revision() [targetting meego-eds branch] (26.87 KB, patch)
2011-07-22 19:23 UTC, Tristan Van Berkom
none Details | Review
Patch to add e_book_client_get_revision() [targetting master] (25.48 KB, patch)
2011-07-22 19:23 UTC, Tristan Van Berkom
none Details | Review
Patch to add e_book_client_get_revision() [targetting master, take 2] (29.05 KB, patch)
2011-07-23 00:13 UTC, Tristan Van Berkom
none Details | Review
Patch to add e_book_get_revision() [targetting meego-eds branch, take 2] (30.13 KB, patch)
2011-07-23 00:45 UTC, Tristan Van Berkom
none Details | Review
Patch to add e_book_client_get_revision() [targetting master, take 3] (29.32 KB, patch)
2011-08-03 18:54 UTC, Tristan Van Berkom
none Details | Review
Patch to add e_book_get_revision() [targetting meego-eds branch, take 3] (29.32 KB, patch)
2011-08-03 18:57 UTC, Tristan Van Berkom
none Details | Review
Patch to add e_book_client_get_revision() [targetting master, take 4] (28.73 KB, patch)
2011-10-13 17:00 UTC, Tristan Van Berkom
needs-work Details | Review
Add the "revision" property to the addressbook backend (11.45 KB, patch)
2011-11-11 02:36 UTC, Tristan Van Berkom
committed Details | Review

Description Murray Cumming 2011-06-09 10:05:37 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.
Comment 1 Tristan Van Berkom 2011-07-16 00:19:18 UTC
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.
Comment 2 Tristan Van Berkom 2011-07-17 22:11:08 UTC
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.
Comment 3 Tristan Van Berkom 2011-07-22 19:20:29 UTC
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
Comment 4 Tristan Van Berkom 2011-07-22 19:23:05 UTC
Created attachment 192476 [details] [review]
Patch to add e_book_get_revision() [targetting meego-eds branch]
Comment 5 Tristan Van Berkom 2011-07-22 19:23:53 UTC
Created attachment 192477 [details] [review]
Patch to add e_book_client_get_revision() [targetting master]
Comment 6 Tristan Van Berkom 2011-07-23 00:13:20 UTC
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).
Comment 7 Tristan Van Berkom 2011-07-23 00:45:34 UTC
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'.
Comment 8 Tristan Van Berkom 2011-08-03 18:54:48 UTC
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
Comment 9 Tristan Van Berkom 2011-08-03 18:57:27 UTC
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
Comment 10 Tristan Van Berkom 2011-10-13 17:00:20 UTC
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()....
Comment 11 Milan Crha 2011-10-31 10:32:19 UTC
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.
Comment 12 Tristan Van Berkom 2011-11-11 02:36:07 UTC
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.
Comment 13 Milan Crha 2011-11-11 07:51:26 UTC
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 :)
Comment 14 Tristan Van Berkom 2011-11-14 15:54:28 UTC
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...)
Comment 15 Milan Crha 2011-11-14 16:36:25 UTC
(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.