Race condition in Store::appendToDelta
mrks opened this issue · 10 comments
I believe that this can lead to a race condition:
std::pair<size_t, size_t> Store::appendToDelta(size_t num) {
std::size_t start =_delta_size.fetch_add(num);
delta->resize(start + num);
[...]
}
While the atomic variable ensures that all appends are reflected in the delta_size, this can lead to a race condition:
_delta_size == 10
T1: appendToDelta(5)
T2: appendToDelta(7)
T1: std::size_t start =_delta_size.fetch_add(num); // start is 10, _delta_size 15
T1 gets preempted
T2: std::size_t start =_delta_size.fetch_add(num); // start is 15, _delta_size 22
T2: delta->resize(15 + 7);
T1: delta->resize(10 + 5);
As we can see, the actual delta size first gets increased to 22, then incorrectly decreased to 17.
Our hotfix is spinlocking the appendToDelta method:
static locking::Spinlock mtx;
std::lock_guard<locking::Spinlock> lck(mtx);
hmmm I think the best thing would be to disallow decreasing the size of a table and thus working append only. The best way would be to catch the resize in Table.cpp and ignore properly.
What do you think?
Yes, that would be one option. Although just adding a spinlock for the append would be way easier and has - until now - never shown up anywhere in my profilings as a bottleneck. So we might go for the easy approach now and if it comes back up as a performance bottleneck we can still redesign it?
This would have to go down all the way to the attribute vectors and the std::vectors. Imagine this situation:
_delta_size == 10
T1: appendToDelta(5)
T2: appendToDelta(7)
T1: std::size_t start =_delta_size.fetch_add(num); // start is 10, _delta_size 15
T1: Delta checks if new size is bigger than previous size - it is
T1 gets preempted
T2: std::size_t start =_delta_size.fetch_add(num); // start is 15, _delta_size 22
T2: delta->resize(15 + 7);
T1 has already done the check and thinks that 15 is bigger than the delta size
T1: delta->resize(10 + 5);
We probably would have to do the fetch_add on all levels.
Sure, for now this should be fine. The only problem might be lots of transactions trying to increase the size of the delta by 1.
There should be then a possibility to reserve even more data and just to fill up the vectors probably... to avoid frequent allocations.
@mrks yes you're right... probably an append only data structure for the tuples would be better. Might be good to think about this rather sooner than later..
This is why the tbb::concurrent_vector-based ConcurrentFixedLengthVector uses grow_to_atleast(...). So there can't be any races in that regard. (Which internally blocks other resizes, IIRC)
grow_to_atleast is not necessarily the same thing. The questions is more, how to perform allocation and reserving of a value range in a single step without using locks.
Well, grow_to_atleast(n) assures that after the call, there is room for at least (n) elements (and this is growth is blocking at the vector level). Also concurrent_vectors cannot be grown to a smaller size - hence the name. Combined with fetch_add, we atomically reserve value ranges. I don't get where we need extra locks here?
grow_to_at_least sounds good.
The potential race on mvcc-vectors was fixed in master. Resizing of the delta itself was already thread-safe beforehand.