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 apiXXXX 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 TSuperDataStore::engProcessRemoteItem(
bool regular=true;
string datatext;
#ifdef SYSYNC_SERVER
- TSyncItem *itemcopyP;
+ TSyncItem *itemcopyP = NULL;
#endif
// show
@@ -617,42 +617,71 @@ bool TSuperDataStore::engProcessRemoteItem(
case sop_delete:
case sop_copy:
// item has no local ID AND no data, only a remoteID:
- // we must try to read item from all subdatastores by remoteID until
- // one is found
+ // we must try to read item or attempt to delete in all subdatastores
by remoteID
+ // until found in one of them
+
// get an empty item of correct type to call logicRetrieveItemByID
itemcopyP =
getLocalReceiveType()->newSyncItem(getRemoteSendType(),this);
// - only remote ID is relevant, leave everything else empty
itemcopyP->setRemoteID(syncitemP->getRemoteID());
- // try to read item from all subdatastores
+ // try to find item in any of the subdatastores
for (pos=fSubDSLinks.begin();pos!=fSubDSLinks.end();pos++) {
linkP = &(*pos);
// always start with 200
aStatusCommand.setStatusCode(200);
- // now try to read
- PDEBUGPRINTFX(DBG_DATA+DBG_DETAILS,(
- "Trying to read item by remoteID='%s' from subdatastore '%s' to
see if it is there",
- itemcopyP->getRemoteID(),
- linkP->fDatastoreLinkP->getName()
- ));
-
regular=linkP->fDatastoreLinkP->logicRetrieveItemByID(*itemcopyP,aStatusCommand);
- // must be ok AND not 404 (item not found)
- if (regular && aStatusCommand.getStatusCode()!=404) {
- PDEBUGPRINTFX(DBG_DATA,(
- "Item found in subdatastore '%s', deleting it there",
+
+ // item deleted by a failed engProcessRemoteItem()?
+ if (!syncitemP) {
+ // recreate it for next attempt
+ syncitemP =
getLocalReceiveType()->newSyncItem(getRemoteSendType(),this);
+ syncitemP->setRemoteID(itemcopyP->getRemoteID());
+ syncitemP->setSyncOp(sop);
+ }
+
+ if (linkP->fDatastoreLinkP->dsDeleteDetectsItemPresence()) {
+ // attempt to delete, consuming original item on success
+
regular=linkP->fDatastoreLinkP->engProcessRemoteItem(syncitemP,aStatusCommand);
+ // must be ok AND not 404 (item not found)
+ if (regular && aStatusCommand.getStatusCode()!=404) {
+ PDEBUGPRINTFX(DBG_DATA,(
+ "Item found in subdatastore '%s', deleted it there",
+ linkP->fDatastoreLinkP->getName()
+ ));
+
+ // done
+ goto done;
+ } else {
+ // syncitemP was deleted by engProcessRemoteItem() (for
+ // example, in TStdLogicDS::logicProcessRemoteItem()),
+ // so we must remember to recreate it from the copy for
+ // another attempt if there is one
+ syncitemP = NULL;
+ }
+ } else {
+ // try to read first to determine whether the subdatastore
contains the item;
+ // necessary because the subdatastore is not able to report a 404
error in
+ // the delete operation when the item does not exist
+ PDEBUGPRINTFX(DBG_DATA+DBG_DETAILS,(
+ "Trying to read item by remoteID='%s' from subdatastore '%s' to
see if it is there",
+ itemcopyP->getRemoteID(),
linkP->fDatastoreLinkP->getName()
));
- // now we can delete or copy, consuming original item
-
regular=linkP->fDatastoreLinkP->engProcessRemoteItem(syncitemP,aStatusCommand);
- // delete duplicated item as well
- delete itemcopyP;
- // done
- regular=true;
- goto done;
+
regular=linkP->fDatastoreLinkP->logicRetrieveItemByID(*itemcopyP,aStatusCommand);
+ // must be ok AND not 404 (item not found)
+ if (regular && aStatusCommand.getStatusCode()!=404) {
+ PDEBUGPRINTFX(DBG_DATA,(
+ "Item found in subdatastore '%s', deleting it there",
+ linkP->fDatastoreLinkP->getName()
+ ));
+ // now we can delete or copy, consuming original item
+
regular=linkP->fDatastoreLinkP->engProcessRemoteItem(syncitemP,aStatusCommand);
+ // done
+ regular=true;
+ goto done;
+ }
}
}
// none of the datastores could process this item --> error
- // - delete duplicated item
- delete itemcopyP;
// - make sure delete reports 200 for incomplete-rollback-datastores
if (aStatusCommand.getStatusCode()==404 && sop!=sop_copy) {
// not finding an item for delete might be ok for remote...
@@ -722,6 +751,9 @@ nods:
regular=false;
goto done;
done:
+ #ifdef SYSYNC_SERVER
+ delete itemcopyP;
+ #endif
PDEBUGENDBLOCK("SuperProcessItem");
return regular;
} // TSuperDataStore::engProcessRemoteItem
diff --git a/src/sysync/syncdatastore.h b/src/sysync/syncdatastore.h
index 78e4559..0672341 100755
--- a/src/sysync/syncdatastore.h
+++ b/src/sysync/syncdatastore.h
@@ -56,6 +56,8 @@ public:
virtual uInt16 isDatastore(const char *aDatastoreURI);
// - returns if specified type is used by this datastore
bool doesUseType(TSyncItemType *aSyncItemType, TTypeVariantDescriptor
*aVariantDescP=NULL);
+ // - true if data store is able to return 404 when asked to delete
non-existent item
+ virtual bool dsDeleteDetectsItemPresence() const { return false; }
// - get common types to send to or receive from another datastore
TSyncItemType *getTypesForTxTo(TSyncDataStore *aDatastoreP, TSyncItemType
**aCorrespondingTypePP=NULL);
TSyncItemType *getTypesForRxFrom(TSyncDataStore *aDatastoreP, TSyncItemType
**aCorrespondingTypePP=NULL);
--
1.7.2.5
_______________________________________________
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis
