Flow directory tester snapshot reads are not RYW

[alecgrieser]

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:

if (isDirectory && isSnapshot) {
    Version readVersion = wait(instruction->tr->getReadVersion());
    instruction->tr = Reference<Transaction>(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.
2. The directories are modified.
3. 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:

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 5fdcd62 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.

[ajbeamon]

There are actually multiple problems with the directory layer testing that I'm tracking as part of my task to clean them up. We should probably just migrate that task into github.

[ajbeamon]

It also looks like I've already implemented a fix for this locally that adds the ability to switch off snapshot reads in the binding layer directory tests, and the flow tester is marked as needing that. I think this is probably the most straightforward resolution until we add snapshot support to the flow directory layer.