CA-171948: Adjust semantics of 'add_to_<map>' in Db layer

[jonludlam]

This allows the DB add_to_map calls to be idempotent. So if a map contains:

[('key','value')]

then

add_to_map(x,'key','value')

will succeed if idempotency is enabled, and fail with Duplicate_key if not.

add_to_map(x,'key','value2')

will always fail with a Duplicate_key exception.

By default, this behaviour is turned on when xapi uses the remote DB access from slaves, but turned off on the master. This has the effect of ensuring that the Xen API calls retain their current semantics for now.

The behaviour can be normalised across master/slave via the config file parameter

db_idempotent_map

which currently defaults to false. This default will change at some point in the future.

[lindig]

Minor quibble: db_idempotent_map - not the map is idempotent but its add operation. Would call it db_map_add_idempotent = true/false.

[jonludlam]

all map operations are idempotent after this c/s, because 'remove' already was.

[lindig]

Some small comments.

[lindig]

I think for performance it would be best to check the cheapest conditions first - that would be idempotent and List.assoc next (I assume).

[jonludlam]

I don't think it's quite so clear cut here - checking the idempotent condition is only relevant if the mem_assoc test is true, and that's expected to be quite rare. Also, in the default case here idempotent is false, hence we don't do the List.assoc.

[lindig]

You are right - I now realise that you do List.mem to avoid a Not_found exception. This can stay as it is. But I think the best performance would have this:

try if not idempotent && List.assoc key t <> value then raise Duplicate 
with Not_found -> ()

[jonludlam]

This doesn't have the same semantics - it doesn't raise any exception if idempotent is true, which isn't the same as the current change.

[lindig]

Last try - not better performance but easier to read maybe:

if List.mem_assoc key t &&  idempotent && List.assoc key t = value then raise Duplicate;

[thomassa]

But that has wrong semantics too.
If idempotent then we shouldn't raise an exception when List.assoc key t = value,
and otherwise we should raise an exception if List.mem_assoc key t regardless of the value.

[lindig]

You are right. I wanted to use: !a || !b = !(a && b) but forgot the outer negation. So it probably should stay the way it is.

[lindig]

I'd consider givingidempotent a label because at a call site true/false isn't very descriptive and easy to mix up. You could even provide a default.

[jonludlam]

Good idea.