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 686678 - Make local addressbook backend use the SQLite DB exclusively
Make local addressbook backend use the SQLite DB exclusively
Status: RESOLVED FIXED
Product: evolution-data-server
Classification: Platform
Component: Contacts
unspecified
Other Linux
: Normal normal
: ---
Assigned To: evolution-addressbook-maintainers
Evolution QA team
Depends on:
Blocks: 687281
 
 
Reported: 2012-10-23 06:57 UTC by Tristan Van Berkom
Modified: 2012-11-08 08:51 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Minor fixup, fixes query terms including uid matching expressions (1.01 KB, patch)
2012-10-29 06:50 UTC, Tristan Van Berkom
committed Details | Review
Add e_book_backend_sqlitedb_set/get_revision() (7.77 KB, patch)
2012-10-29 06:52 UTC, Tristan Van Berkom
accepted-commit_now Details | Review
Make local backend use EBookBackendSQLiteDB exclusively (72.36 KB, patch)
2012-10-29 06:54 UTC, Tristan Van Berkom
reviewed Details | Review
Fixed syntax errors in previous patch (9.82 KB, patch)
2012-11-07 07:33 UTC, Tristan Van Berkom
committed Details | Review
Updated patch to make local backend use the SQLiteDB exclusively (72.42 KB, patch)
2012-11-07 07:37 UTC, Tristan Van Berkom
committed Details | Review

Description Tristan Van Berkom 2012-10-23 06:57:52 UTC
Currently the local file backend of EDS uses Berkeley DB for storing plain 
vCards, indexed by UID and additionally it uses the SQLite database
(EBookBackendSqliteDB) for summaries, which avoids parsing plain vCards
on every query and therefore improves performance of queries.

However, using two nested databases leads to unnecessary I/O and contributes
to flash wear. It would be nice to reduce disk writes by removing the
Berkeley DB interactions entirely, instead storing the plain vCard directly
in the SQLite backend.

As a side effect, this could clean up existing code and reduce the overall 
complexity of the local addressbook backend.

Will provide proposed patches for this shortly.
Comment 1 Tristan Van Berkom 2012-10-29 06:50:52 UTC
Created attachment 227502 [details] [review]
Minor fixup, fixes query terms including uid matching expressions

This is just an additional patch to the ones I'm adding here, basically it's a non issue if you use the get_contact by id apis, but using query terms that include the E_CONTACT_UID breaks without this...
Comment 2 Tristan Van Berkom 2012-10-29 06:52:14 UTC
Created attachment 227503 [details] [review]
Add e_book_backend_sqlitedb_set/get_revision()

In order for the file backend to use SQLiteDB as a complete store, it needs somewhere to store the overall addressbook revision

This patch extends EBookBackendSQLiteDB to include a column for the revision
Comment 3 Tristan Van Berkom 2012-10-29 06:54:26 UTC
Created attachment 227504 [details] [review]
Make local backend use EBookBackendSQLiteDB exclusively

This patch is the juice, and depends on the included patch for storing the
revision in the sqlite.

I've run some of the regression test on master with this applied and no problems
in sight.
Comment 4 Tristan Van Berkom 2012-10-29 06:58:45 UTC
Note... I've revived the 'openismus-work-master' branch and brought it up to speed.

All of these patches can equally be found on that branch.
Comment 5 Milan Crha 2012-10-29 12:35:58 UTC
Thanks for a bug report and a great job with the cleanup. Once an EBackendOfflineCache is written it'll replace the SQLiteDB cache, as it'll be an extended version of it, but that's far away, thus good you dive into it.

The patches look good, if I had to pick a little nitpick, then it might be at the e_book_backend_file_upgrade_db(), the old_version not being defined as 'const', but I see it was there before your changes, thus feel free to change it or keep as is, I'm fine with it.

I thought there is no issue with the patches, but I found some:
a) if the old adressbook is empty, then the open of it claims an error about failed open, and the detailed error says: "Cannot open book: near "TABLE": syntax error". I get the same error after restart of all evolution's processes on the Personal book.

b) if I create a new book, then I cannot do with it anything, like adding contacts to it. It doesn't claim any error in UI, neither on evolution's console, only the options are not available.
Comment 6 Tristan Van Berkom 2012-11-07 05:50:59 UTC
Hi, I've been spending last week getting ahead and hacking on other patches
and now I've come back to this...

So, yesterday I tried to reproduce your problems in a test case; I tried
to
  a.) Create a new book
  b.) Close the new book without adding any contacts to it
  c.) Open the same book
  d.) Add a contact
  e.) Fetch the added contact.

But I was not able to reproduce any error.

So this morning I proceeded to compile the Evolution Mail program
from master.

I started up the source registry, addressbook and calendar services
with XDG_*_HOME dirs pointing somewhere custom... and then fired up
evolution mail with the same XDG dirs

  o Went through the steps of adding an initial email
  o Checked the addressbook in the Evo UI
  o Closed the UI without adding any contacts
  o Restarted all EDS processes and restarted Evo

Up to here I still don't get the "Cannot open book:..." error

  o Added a new addressbook (by right clicking in the treeview and
    using the "New Address Book" context menu option).
  o Added contacts to the default "Personal" address book (by using the
    "New" button in the main toolbar)
  o Added contacts to the "Unnamed" address book I had previously
    created

The new contacts appear in the right hand area, I can select them
and view the details I previously entered in the dialog.

The one error I do get is when actually committing a new contact
(i.e. when pressing OK in the "Add New Contact" dialog), I get
this warning message:

  "You are attempting to move a contact from one address book to another 
   but it cannot be removed from the source.
   Do you want to save a copy instead?"

I'm quite sure this is unrelated, in any case I just select "Yes" and
the new contact is added without any trouble.

Any further tips on how to reproduce this problem ?

I'll try compiling EDS without the BDB removal and experiment with
the cross-over.
Comment 7 Tristan Van Berkom 2012-11-07 07:33:51 UTC
Created attachment 228338 [details] [review]
Fixed syntax errors in previous patch

This is the same patch with the SQL syntax error upon upgrading out of date DB fixed.

Changed:
  UPDATE TABLE folders SET version = 2
to:
  UPDATE folders SET version = 2
Comment 8 Tristan Van Berkom 2012-11-07 07:37:20 UTC
Created attachment 228339 [details] [review]
Updated patch to make local backend use the SQLiteDB exclusively

This second version of the patch fixes 2 things:

  o Avoid calling e_book_backend_sqlitedb_add_contacts() with a null contact list
    (this was causing an assertion)

  o Fixed the migration routine to open the BDB with write access

    This is needed in order to maintain the previous portions of the BDB 
    upgrades, i.e. when an older BDB version is encountered we use the same
    older upgrade paths to arrive at the current version, and then afterwards
    migrate those contacts to the SQLite DB.
Comment 9 Tristan Van Berkom 2012-11-07 07:47:50 UTC
Milan,
   I spent the time to debug the scenario and did reproduce the errors you
found while using the Evolution Mail client

I used 2 scenarios to reproduce & test with:

  o Compiled Evolution/EDS current master
  o Ran Evolution (and EDS processes) with relocated XDG_DATA_HOME,
    XDG_CONFIG_HOME & XDG_CACHE_HOME (all pointing to ~/tmp_evo)
  o Accessed the "Contacts" page in the UI and did not add any contacts
  o Stopped Evolution & EDS processes
  o Copied the contents of ~/tmp_evo/evolution into a backup directory
  o Started Evolution & EDS processes again
  o Created some test contacts as well as an extra test addressbook
  o Stopped Evolution & EDS processes
  o Copied the contents of ~/tmp_evo/evolution into another backup directory

At this point I had good samples of what Evolution creates for both
an empty and populated addressbook... so I proceeded to

  o Compile & install openismus-work-master branch again
  o Fire up Evolution & EDS processes again and encounter the errors
    of which you spoke

After some debugging I have the migration working cleanly:
  o Migrating from an empty addressbook works, can add contacts after
    the migration
  o Migration from my populated test addressbook also works, the contacts
    I added to the "Personal" and added "Custom" addressbook appear in
    the Evolution UI after the migration takes place.

I've updated attached patches with the relevant fixes.

However there remains this one sticky thing, the warning I spoke of
in my first comment today:
  "You are attempting to move a contact from one address book to another 
   but it cannot be removed from the source.
   Do you want to save a copy instead?"

Still appears... I have really no clue where that error is coming from.
Comment 10 Milan Crha 2012-11-07 09:36:06 UTC
Comment on attachment 228338 [details] [review]
Fixed syntax errors in previous patch

Only fix the C++ comment (//), change it to C comment style (/* */).
If you want, then I would not add two new line breaks after declaration of e_book_backend_sqlitedb_set_revision(), but that's really minor thing.
Comment 11 Milan Crha 2012-11-07 10:18:16 UTC
Comment on attachment 228339 [details] [review]
Updated patch to make local backend use the SQLiteDB exclusively

Do not forget to turn off d(x) in e-book-backend-file.c before committing.

e_book_backend_file_open() has set readonly=TRUE at is beginning, then it's never changed, thus the books are readonly, which explains the warning on contact save.

Otherwise looks good, please commit
Comment 12 Tristan Van Berkom 2012-11-08 05:48:45 UTC
Patches committed in master, I suppose we can close this one now.