aboutsummaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorDaniel Vrátil <dvratil@kde.org>2016-08-26 12:52:57 (GMT)
committerDaniel Vrátil <dvratil@kde.org>2016-08-26 12:52:59 (GMT)
commit9d1d7cdff89b6a0cd4b77fa83b59830279fe117f (patch)
tree078f100ac5f3f65598d5f270e3fc9003d8cdaeac
parent34c1f14836f4d0f04b2175b70b4073384a522b16 (diff)
MOVE: fix concurrency bug and improve performance on complex moves
The MOVE handler would emit the notification before storing the updated data in the database. Sometimes this has lead to the resource getting the notification and fetching the Item before the Server would update it in the database. Specifically in case of IMAP resource this would hit a code path that checks whether the moved Item has a correct parent set and discard the change if not. This patch makes the changes to be stored in DB right away and the notification is emitted after that. It also does only a single iteration over the whole list of Items to be moved, which should be faster in case of batched moves. Finally it uses just a single SQL query to reset RID of all items that were moved between resources instead of udpating each Item individually.
-rw-r--r--src/server/handler/move.cpp66
1 files changed, 42 insertions, 24 deletions
diff --git a/src/server/handler/move.cpp b/src/server/handler/move.cpp
index 5bef80e..05182ef 100644
--- a/src/server/handler/move.cpp
+++ b/src/server/handler/move.cpp
@@ -55,7 +55,12 @@ void Move::itemsRetrieved(const QList<qint64> &ids)
QMap<Entity::Id /* collection */, PimItem> toMove;
QMap<Entity::Id /* collection */, Collection> sources;
Q_FOREACH (/*sic!*/ PimItem item, items) {
- const Collection source = items.at(0).collection();
+ if (!item.isValid()) {
+ failureResponse("Invalid item in result set!?");
+ return;
+ }
+
+ const Collection source = item.collection();
if (!source.isValid()) {
failureResponse("Item without collection found!?");
return;
@@ -64,10 +69,6 @@ void Move::itemsRetrieved(const QList<qint64> &ids)
sources.insert(source.id(), source);
}
- if (!item.isValid()) {
- failureResponse("Invalid item in result set!?");
- return;
- }
Q_ASSERT(item.collectionId() != mDestination.id());
item.setCollectionId(mDestination.id());
@@ -78,35 +79,52 @@ void Move::itemsRetrieved(const QList<qint64> &ids)
item.setDirty(true);
}
+ if (!item.update()) {
+ failureResponse("Unable to update item");
+ return;
+ }
+
toMove.insertMulti(source.id(), item);
}
// Emit notification for each source collection separately
- Q_FOREACH (const Entity::Id &sourceId, toMove.uniqueKeys()) {
- PimItem::List itemsToMove;
- for (auto it = toMove.cbegin(), end = toMove.cend(); it != end; ++it) {
- if (it.key() == sourceId)
- itemsToMove.push_back(it.value());
+ QVector<PimItem::Id> itemsToResetRID;
+ Collection source;
+ PimItem::List itemsToMove;
+ for (auto it = toMove.cbegin(), end = toMove.cend(); it != end; ++it) {
+ if (source.id() != it.key()) {
+ if (!itemsToMove.isEmpty()) {
+ store->notificationCollector()->itemsMoved(itemsToMove, source, mDestination);
+ }
+ source = sources.value(it.key());
+ itemsToMove.clear();
}
- const Collection &source = sources.value(sourceId);
- store->notificationCollector()->itemsMoved(itemsToMove, source, mDestination);
+ itemsToMove.push_back(*it);
- for (auto iter = toMove.find(sourceId), end = toMove.end(); iter != end; ++iter) {
- // reset RID on inter-resource moves, but only after generating the change notification
- // so that this still contains the old one for the source resource
- const bool isInterResourceMove = (*iter).collection().resource().id() != source.resource().id();
- if (isInterResourceMove) {
- (*iter).setRemoteId(QString());
- }
+ // reset RID on inter-resource moves, but only after generating the change notification
+ // so that this still contains the old one for the source resource
+ const bool isInterResourceMove = it->collection().resource().id() != source.resource().id();
+ if (isInterResourceMove) {
+ itemsToResetRID.push_back(it->id());
+ }
+ }
- // FIXME Could we aggregate the changes to a single SQL query?
- if (!(*iter).update()) {
- failureResponse("Unable to update item");
- return;
- }
+ if (!itemsToMove.isEmpty()) {
+ store->notificationCollector()->itemsMoved(itemsToMove, source, mDestination);
+ }
+
+ // Batch-reset RID for all inter-resource moves
+ if (!itemsToResetRID.isEmpty()) {
+ QueryBuilder qb(PimItem::tableName(), QueryBuilder::Update);
+ qb.setColumnValue(PimItem::remoteIdColumn(), QString());
+ ItemQueryHelper::itemSetToQuery(itemsToResetRID, connection()->context(), qb);
+ if (!qb.exec()) {
+ failureResponse("Unable to update RID");
+ return;
}
}
+
} else {
failureResponse("Unable to execute query");
return;