Flow directory tester snapshot reads are not RYW
- Dominant language
- C++
- Stars
- 16.7k
- Forks
- 1.6k
- Avg merge
- 1d 20h
- Merged PRs (30d)
- 126
Description
Right now, the flow directory layer does not support snapshot reads. This in and of itself is a shortcoming of the flow bindings, but it is not so bad really. The flow binding tester attempts to get around this with the following bit of code:
```c++
if (isDirectory && isSnapshot) {
Version readVersion = wait(instruction->tr->getReadVersion());
instruction->tr = Reference(new Transaction(data->db));
instruction->tr->setVersion(readVersion);
}
```
The problem is that though this ensures that the "snapshot" transaction has the same view of the database as the regular transaction, it is missing read-your-writes support. This means that the tests can fail if there is something along the lines of:
1. Transaction starts.
1. The directories are modified.
1. The directories are read at snapshot isolation level.
The flow tester will miss the modifications, so it will print out different results.
I ran into this error when running the tests locally. Here is the input that I can give to the bindingtester to make it reproduce:
```bash
python bindingtester.py --test-name directory --num-ops 261 flow --seed 3583312077 --compare
```
(The number of ops, 261, is the minimum needed to get it to reproduce with that seed.) I was running it against commit 5fdcd6278b25877038efa0bd8e5e77c354691bee which, at the time, was the tip of the release-5.2 branch, to the extent that that is important for reproduction. Here is a print out of the relevant section of adding `--print-test --all` to the command:
```
2175. 'RESET'
2176. 'PUSH' 0
2177. 'DIRECTORY_REMOVE_DATABASE'
2178. 'COMMIT'
2179. 'WAIT_FUTURE'
2180. 'RESET'
2181. 'PUSH' 3
2182. 'DIRECTORY_CHANGE'
2183. 'PUSH' 'default-6\x1e\xde-M\\R:\xea\x19\xdd\xe7\xa3\x96\xaa\x9d'
2184. 'PUSH' ''
2185. 'PUSH' u'default1'
2186. 'PUSH' 1
2187. 'DIRECTORY_CREATE_DATABASE'
2188. 'PUSH' 4
2189. 'DIRECTORY_CHANGE'
2190. 'PUSH' u'1'
2191. 'PUSH' 1
2192. 'PUSH' 1
2193. 'DIRECTORY_REMOVE'
2194. 'PUSH' u'\u70c1\uc3f9\ufe98\u34e1\U0002a2b2\uffa3\u6283\u5f6a\u5a34\ufc6c\u7526\u3df9\u0c1e\u6b44\u532e\u7269\u2d1c\ub766\u583b\uacd9\u3b0d\uc34e\u4218\u21f4'
2195. 'PUSH' ()
2196. 'PUSH' 2
2197. 'DIRECTORY_PACK_KEY'
2198. 'DIRECTORY_UNPACK_KEY'
2199. 'PUSH' 27
2200. 'DIRECTORY_CHANGE'
2201. 'PUSH' '\x13\x11\x05'
2202. 'PUSH' 0
2203. 'DIRECTORY_CREATE_SUBSPACE'
2204. 'PUSH' 97
2205. 'DIRECTORY_CHANGE'
2206. 'PUSH' u'2'
2207. 'PUSH' u'3'
2208. 'PUSH' u'3'
2209. 'PUSH' 3
2210. 'PUSH' 0
2211. 'DIRECTORY_MOVE'
2212. 'PUSH' 0
2213. 'PUSH' 1
2214. 'DIRECTORY_LIST_SNAPSHOT'
```
This leads to the following discrepancy between flow and python:
```
Incorrect result:
flow - ('tester_output', 'stack', 269, 2214) = '\x01\x021\x00\xff\x00'
python - ('tester_output', 'stack', 269, 2214) = '\x01\x00'
```
I hacked up the binding tester to actual emit non-snapshot reads whenever it thinks it should actually emit a snapshot read, and then the test passed. So, I don't think it's actually a bug in the directory layer, just that the lack of RYW in snapshot tests.
This can cause spurious errors, so it would be nice to get it fixed. In some sense, the "correct" solution is to make the flow directory layer support snapshot reads. I'm not sure if there is an easier way. In theory, we could just give the snapshot tester a regular transaction. This can lead to a different kind of spurious error (additional `not_committed` errors), so that's not air-tight. In theory, we could also log all of the writes and replay them within the snapshot transaction or do something like keep two transactions around and use both of them for writes, but perform non-snapshot reads and commits on one and "snapshot" reads on the other. But this might be harder than just supporting snapshot reads.
Contributor guide
Research direction
Start with bindingtester.py and the Flow binding tester's snapshot-transaction setup, then reproduce the discrepancy with the directory command and seed given in the issue. Determine how snapshot directory reads can retain read-your-writes behavior; done means the reproduction no longer produces different Flow and Python results.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- cpp, python
- Domain
- databases, testing-qa
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Needs clarification
- Newbie friendliness
- 35/100