Re: [os-libsynthesis] temporary UID mapping

2010-12-09 Thread Patrick Ohly
On Do, 2010-12-09 at 11:26 +, Lukas Zeller wrote:
> Still, the thing is that there's never a deletion of a single tempGUID
> map entry (of course, there ARE deletions of map entries with other
> types, but not of mapentry_tempidmap).

Does that type map to sysync::cMapID::ident?

In my mapping on disk I currently see only entries with ident == 2 and a
a remote ID.

So you are saying that entries with ident == 2 are never meant to be
removed from the mapping? Won't that overflow the disk after (some
admittedly rather long) usage with many new items transferred?

> If you see the number of mapentry_tempidmaps decreasing during a sync
> session (counted from that anchorpoint at line 5852 in
> localengineds.cpp), there must be a problem with making these
> persistent and loading them later on (or some other not yet discovered
> bug - in the engine or the DB plugins). 

There definitely were missing entries. I'll keep watching :-/

-- 
Best Regards, Patrick Ohly

The content of this message is my personal opinion only and although
I am an employee of Intel, the statements I make here in no way
represent Intel's position on the issue, nor am I authorized to speak
on behalf of Intel on this matter.



___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis


Re: [os-libsynthesis] temporary UID mapping

2010-12-09 Thread Lukas Zeller
Hello Patrick,

On Dec 8, 2010, at 12:22 , Patrick Ohly wrote:

> The Synthesis engine has the feature that its backends are allowed to
> use IDs of arbitrary length. The engine will translate into IDs shorter
> than the maximum ID size supported by the peer.

It only works for the server, which makes sure it does not send IDs longer than 
what the client allows. The server must be able to handle whatever size of IDs 
a client might have.

This is plain SyncML requirements - when the standard was defined the idea was 
that there might be clients with resources so limited that they might not be 
able to store the often longer server IDs. The opposite - a server with a too 
short remoteID DB field was considered unlikely and the standard does not 
provide a mechanism for that case (and libsynthesis doesn't, either).

> TLocalEngineDS::adjustLocalIDforSize() creates these temporary IDs,
> using:
>  fTempGUIDMap.size() + 1
>  // as list only grows, we have unique tempuids for sure
> 
> I'm currently (involuntarily ;-) stress-testing this code by running
> SyncEvolution<->SyncEvolution syncs with lots of iCalendar 2.0 items,
> which happen to have very long IDs.
> 
> I see failures where the server assigns the same temporary ID to
> different items in the same sync.
> 
> I've added debug logging. It shows that the following happens:
> 1. fTempGUIDMap is restored from the map file such that it has 105
>entries, *but* these are non-contiguous (from #1 to #124).
> 2. fTempGUIDMap.size() + 1 is #106, which already exists in
>fTempGUIDMap.
> 3. Overwriting that entry does not increase fTempGUIDMap.size().
> 4. A second item is assigned the same #106 temp ID.
> 
> I don't know exactly how I arrived at this state. Is the on-disk dump of
> fTempGUIDMap really meant to preserve all temporary IDs in perpetuity?

No. The life span of a temporary ID is one complete sync, with the possibility
of "pending mappings" carried over to the beginning of the next session.
But before that next session actually starts, all existing tempIDs become 
invalid
(Line 5852 in localengineds.cpp) and new ones needed in the progress of the
sync can use the same values again.

> I don't think so, because that would fill up the disk and there is a
> "delete map item" operation.
> 
> Unless there is some additional constraint, then the assumption that
> "the list only grows" is wrong.

I still think the assumption is correct. I admit the way this works is a bit 
fragile because the , so no objections to make it more robust!

Still, the thing is that there's never a deletion of a single tempGUID map 
entry (of course, there ARE deletions of map entries with other types, but not 
of mapentry_tempidmap). It starts with an empty fTempGUIDMap container at the 
beginning of the sync, and then for each  sent to the client where the 
actual localID is longer than the maxGUIDsize of the client, a new tempGUID map 
entry is created. A session might get suspended several times, which means that 
all map entries, including the contents of fTempGUIDMap might need to be made 
persistent in the DB and restored into a new session instance several times as 
well, but during all this time IMHO no map entry of type mapentry_tempidmap is 
deleted.

If you see the number of mapentry_tempidmaps decreasing during a sync session 
(counted from that anchorpoint at line 5852 in localengineds.cpp), there must 
be a problem with making these persistent and loading them later on (or some 
other not yet discovered bug - in the engine or the DB plugins).


> I have added a workaround which checks
> for ID collisions in TLocalEngineDS::adjustLocalIDforSize(). Is that the
> right solution or do I need to search for the reason why the mapping has
> gaps?

It makes the whole thing more robust for the case that for some future change 
in the engine the "increasing list" assumption could become invalid, but for 
now I guess the workaround will just hide another problem elsewhere. So I'd add 
a big fat red DBG_ERROR type message when you detect a collision, so we'll find 
if this happens regularily and if so, we can find out why.

Best Regards,

Lukas


___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis


Re: [os-libsynthesis] temporary UID mapping

2010-12-09 Thread Lukas Zeller
Hello Patrick,

On Dec 9, 2010, at 13:24 , Patrick Ohly wrote:

> On Do, 2010-12-09 at 11:26 +, Lukas Zeller wrote:
>> Still, the thing is that there's never a deletion of a single tempGUID
>> map entry (of course, there ARE deletions of map entries with other
>> types, but not of mapentry_tempidmap).
> 
> Does that type map to sysync::cMapID::ident?

Yes.

> In my mapping on disk I currently see only entries with ident == 2 and a
> a remote ID.

2 = is mapentry_tempidmap.

After a successful sync to a non-empty client, I'd expect some ident==1 entries 
(= mapentry_normal = real SyncML remoteID<->localID map entries) as well.

ident==3 (= mapentry_pendingmap) entries only exist on a client after a 
suspended or aborted sync which had s from the server.

> So you are saying that entries with ident == 2 are never meant to be
> removed from the mapping?

Not exactly that - what I'm saying is that these are (or should) always be 
removed all together. So either the list is still growing, or all of its items 
should go away at the same time, which cleans up the ID space.

>From a DB plugin perspective, you should always see a row of DeleteMapItem 
>operations, and after these are executed no ident==2 items should be left in 
>the database.

I checked through the code that issues these DeleteMapItem operations, and I 
don't see how something else could happen. The mechanism is not trivial because 
there's a level in between fTempGUIDMap and the actual load/save (fMapTable) to 
optimize DB accesses (prevent deleting and re-adding entries in the DB if 
mappings just get temporarily deleted and re-added between load and next save). 
Maybe there's a condition that can cause some, but not all ident==2 entries to 
become deleted.

> Won't that overflow the disk after (some
> admittedly rather long) usage with many new items transferred?

That would defintely be a bad thing ;-)

Lukas
___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis


Re: [os-libsynthesis] temporary UID mapping

2010-12-09 Thread Patrick Ohly
On Do, 2010-12-09 at 13:18 +, Lukas Zeller wrote:
> > So you are saying that entries with ident == 2 are never meant to be
> > removed from the mapping?
> 
> Not exactly that - what I'm saying is that these are (or should)
> always be removed all together. So either the list is still growing,
> or all of its items should go away at the same time, which cleans up
> the ID space.

I might have found it.

Scenario:
- SERVER is the ID on the server, CLIENT on the client
- server has a mapping from SERVER to #35 (ident 2) and to CLIENT (ident 1)
- item is deleted on server
- two-way sync

What I now see in the log is this:
fTempGUIDMap: restore mapping from #35 to SERVER
Item LocalID='SERVER', RemoteID='CLIENT', operation=delete
SyncOp=delete: LocalID=SERVER RemoteID=CLIENT
fTempGUIDMap: translated realLocalID='SERVER' to tempLocalID='#63'

The fTempGUIDMap is a map tempLocalID->ID. It now contains #65->SERVER and 
#35->SERVER.

But the map in the DB AP is a map from (ID, ident) -> ([remote ID], flags).
It cannot hold both #65->SERVER and #35->SERVER. So what I see next is the 
saving of these two entries:

ModifyMap called: aEntryType=tempidmap, aLocalID='SERVER, aRemoteid='#35', 
aMapflags=0x0, aDelete=0
found entry by entrytype/localID='SERVER' - remoteid='#35', mapflags=0x0, 
changed=0, deleted=1, added=0, markforresume=0, savedmark=0
ModifyMap called: aEntryType=tempidmap, aLocalID='SERVER, aRemoteid='#63', 
aMapflags=0x0, aDelete=0
found entry by entrytype/localID='SERVER' - remoteid='#35', mapflags=0x0, 
changed=0, deleted=0, added=0, markforresume=0, savedmark=0
apiSaveAdminData: entryType=normal, localid='SERVER', remoteID='CLIENT', 
mapflags=0x8, changed=0, deleted=0, added=0, markforresume=1, savedmark=0
UpdateMapItem 'SERVER' + 1 = 'CLIENT' + 9, res=0
apiSaveAdminData: entryType=tempidmap, localid='SERVER', remoteID='#63', 
mapflags=0x0, changed=1, deleted=0, added=0, markforresume=0, savedmark=0
UpdateMapItem 'SERVER' + 2 = '#63' + 0, res=0

The last UpdateMapItem overwrites the entry for #35, because it uses the
same ID + ident.

The next sync then populates fTempGUIDMap with entry #63, but has no #35
and thus the size() + 1 assumption no longer holds.

I remember that I was confused about the map item DB calls. Originally I
implemented that as (ID) -> (ident, [remote ID], flags). But after we
discussed it, I changed that to (ID, ident) -> ([remote ID], flags),
which IMHO is what the API description says:

/*! Map table handling: Update a map item of this context
 *
 *  @param The datastore context
 *  @param  MapID ( with \,\, \ and 
\ ).
 *   If there is already a MapID element with \ 
and \,
 *   it will be updated, else created.
 *
 *  @return  error code, if this MapID can't be updated (e.g. not yet existing).
 *
 *  USED ONLY WITH \
 */
_ENTRY_ TSyError UpdateMapItem( CContext aContext, cMapID mID );

So, what is the solution to this problem? Do I still misunderstand
something or is there a genuine problem? ;-/

FWIW, there was one DeleteMapItem for item SERVER in the sessions above,
but it was for ident 1 (after deleting it).

-- 
Best Regards

Patrick Ohly
Senior Software Engineer

Intel GmbH
Open Source Technology Center   
Pützstr. 5  Phone: +49-228-2493652
53129 Bonn
Germany


___
os-libsynthesis mailing list
os-libsynthesis@synthesis.ch
http://lists.synthesis.ch/mailman/listinfo/os-libsynthesis