On Tue, 2011-10-25 at 09:24 +0200, Lukas Zeller wrote:
On Oct 24, 2011, at 16:55 , Patrick Ohly wrote:
Why is it necessary to read before trying to delete? If the item exists,
then reading it is a fairly expensive test.
Unfortunately, with some some backends, this is the only test that reliably
works. For example, some SQL databases (or their ODBC driver) can't return a
correct number of affected rows for DELETE statements. So reading before
deleting was the only way to detect if any of the subdatastores really had
that item and correctly return 404 or 200.
So far, my backends were written with the expectation that they have to
cope with delete requests for items which are already deleted. This
follows from the inherent race condition between syncing and some other
process which might delete items while a sync runs.
Are all backends expected to return 404 when an item doesn't exist?
Not all backends (see above, built-in ODBC can't), but yes, for plugin
backends, returning proper error codes is specified, also for delete.
Still, if a plugin would not conform to that in its implementation of
delete, that would probably have gone unnoticed so far.
I checked the API docs and did not see it mentioned explicitly.
In SyncEvolution I added some remarks about 404 (will probably not be
noticed) and also added automated Client::Source tests for it (which is
harder to ignore if someone runs the tests ;-)
I also changed the error logging so that 404 is reported to users in
ReadItem and not logged in DeleteItem (while still returned to the
engine).
Of course, the test is a bit expensive - if that's a concern, one
could differentiate between plugin datastores and others (all the
builtin ones, including SQL/ODBC), and use the expensive read test
only for the latter. Like a virtual dsDeleteDetectsItemPresence()
which returns false by default, but can be overridden in plugin
datastores to return true.
The patch below implements that idea (from the for-master/bmc23744
branch). SyncEvolution relies on that patch to avoid the (harmless)
ERROR messages for item not found in ReadItem(), although everything
should work okay also without the patch.
Does that look right?
From fb5cc0dc19fb60e40a4ca2dcfe44a52a75ec7354 Mon Sep 17 00:00:00 2001
From: Patrick Ohly patrick.o...@intel.com
Date: Fri, 28 Oct 2011 15:42:57 +0200
Subject: [PATCH] server engine: more efficient deletion in superdatastore
Attempting to read an item just to check whether it exists is expensive.
It also may trigger error messages when the item does not exist (done by
SyncEvolution).
Therefore, if a data store is able to properly report whether it found
the item which is to be deleted, a different logic is used for
deletion in the superdatastore:
- attempt to delete until it succeeds in a subdatastore
- recreate the sync item from the copy after a failed delete attempt
(which deletes the sync item), if there is another loop iteration
Deleting the copy of the sync item was moved to the function exit code
to avoid code duplication.
By default all data stores are assumed to not support 404 in its
delete operation. The only exception is the plugin API. The (somewhat
undocumented) assumption is that all methods properly report 404 when
the requested item does not exist. The attempt to read code already
relied on that. Now the code relies on that for the attempt to
delete.
Probably it even works when the plugin returns some other error
code (the regular value will be false in that case) or no error code
at all: the translation code between remote ID and local ID will
already detect that the item is not in the mapping table if it is not
in the subdatastore. So the actual plugin will not get called at all
in the case where its expected behavior would matter.
---
src/DB_interfaces/api_db/pluginapids.h |4 ++
src/sysync/superdatastore.cpp | 80 ++--
src/sysync/syncdatastore.h |2 +
3 files changed, 62 insertions(+), 24 deletions(-)
diff --git a/src/DB_interfaces/api_db/pluginapids.h
b/src/DB_interfaces/api_db/pluginapids.h
index 4fbfc79..f215094 100755
--- a/src/DB_interfaces/api_db/pluginapids.h
+++ b/src/DB_interfaces/api_db/pluginapids.h
@@ -172,6 +172,10 @@ public:
virtual void dsResetDataStore(void) { InternalResetDataStore();
inherited::dsResetDataStore(); };
virtual ~TPluginApiDS();
+ // override TSyncDataStore: the plugin must be able to return 404
+ // when an item is not found during delete
+ virtual bool dsDeleteDetectsItemPresence() const { return true; }
+
#ifndef BINFILE_ALWAYS_ACTIVE
/// @name api methods defining the interface from TCustomImplDS to
TXXXApi actual API implementations
/// @{
diff --git a/src/sysync/superdatastore.cpp b/src/sysync/superdatastore.cpp
index e70d7bb..5c0a2b5 100755
--- a/src/sysync/superdatastore.cpp
+++ b/src/sysync/superdatastore.cpp
@@ -530,7 +530,7 @@ bool
