vinyl: simplify read iterator restoration behavior
The 'restore' method, which is implemented by txw, cache, and memory sources, restores an iterator position after a yield in case the source has changed. It is passed the last key and it is supposed to position the iterator to the statement following the last key provided it had to reposition the iterator at all. For all kinds of sources, it first checks the source version and bails out early if it hasn't changed since the last time the iterator was used. If it has changed, it either looks up the statement following the given last key or tries to advance the iterator statement by statement (in case of a cache iterator, to avoid extra cache lookups). Currently, the method returns 1 if the iterator position has actually changed as a result of the call, 0 otherwise. That is, it returns 0 even if the version has changed and it had to perform a full lookup, but landed on the same statement. This is confusing, because in this case the caller has to check if it has to advance the iterator even though it doesn't need to, as the iterator is already positioned at the next key - see vy_read_iterator_scan_* family of functions. Let's simplify the restoration rules - make the 'restore' method return 1 if it had to restore the iterator position irrespective of whether the position has actually changed or not. This means that in case the method returns 1, the caller knows that the iterator is already positioned properly and doesn't need to be advanced. If it returns 0, it means that the iterator is positioned at the same statement as before and we need to check if we need to advance it. This simplifies the iterator code by removing position checking, which would turn real nasty once multikey indexes are introduced. Note, comments to 'restore' methods already conform to this behavior (they weren't quite correct before this patch). There's a catch though - vy_read_iterator_restore_mem relies on the old behavior to skip the cache source in case the current key gets updated during yield. However, it's easy to fix that without introducing any extra overhead - we just need to check if the cache source is at the front of the iterator and clear its history if it is. BTW it turned out that this behavior wasn't covered by tests - when I removed the line invalidating the cache source from vy_read_iterator_restore_mem, all tests passed as usual. So while we are at it, let's also add a test case covering this piece of code.
Showing
- src/box/vy_cache.c 10 additions, 7 deletionssrc/box/vy_cache.c
- src/box/vy_mem.c 0 additions, 3 deletionssrc/box/vy_mem.c
- src/box/vy_read_iterator.c 3 additions, 1 deletionsrc/box/vy_read_iterator.c
- src/box/vy_tx.c 0 additions, 3 deletionssrc/box/vy_tx.c
- test/vinyl/stat.result 2 additions, 2 deletionstest/vinyl/stat.result
- test/vinyl/upsert.result 44 additions, 0 deletionstest/vinyl/upsert.result
- test/vinyl/upsert.test.lua 18 additions, 0 deletionstest/vinyl/upsert.test.lua
Loading
Please register or sign in to comment