Thank you for the explanation Danny. Indeed I didn't fix what you described. That could be done easily by wrapping the handler with WITH-DB-CRITICAL-SECTION. I'm not sure about the consequences in terms of performance, given that this will send a huge function to a channel, and that all the work will be done in the same thread. If you think it's worth it, don't hesitate to send a patch.
Clément Danny Milosavljevic <dan...@scratchpost.org> writes: > Hi Clément, > > I've read through the patch and it seems to handle the case I mean fine > because > you support an arbitrary number of queries per db critical section - so I > agree > that this patchset is fine. > > Keep in mind this is only fine if the critical section is held over an entire > http > request handler and not only over a single database query (as far as I can see > the former is the case in the patch - OK). > > Much longer explanation follows: > > On Mon, 27 Aug 2018 15:18:09 +0200 > Clément Lassieur <clem...@lassieur.org> wrote: > >> Danny Milosavljevic <dan...@scratchpost.org> writes: >> >> > Hi Clément, >> > >> > in the future I plan on making the actual bin/evaluate use another >> > database connection >> > in order for the web interface to be isolated while it's querying. >> >> I don't understand... bin/evaluate doesn't query the database at all. I >> don't know why it would need to. > > Yeah, it has moved. Sorry. > > But I mean the part that changes the values in the database (on behalf of > bin/evaluate). > So now it's the procedure "evaluate" in src/cuirass/base.scm . > >> > Otherwise - as it is now in master - it can happen that while you are >> > querying one >> > page, half of the things have different values than you requested - which >> > is really >> > weird. >> > >> > For example right now you could query for a row with status=42 and get >> > back data with >> > status=43 (because it has been changed in the mean time). >> >> Could you please show an example that I can reproduce? I don't >> understand what you mean. > > Right now something like this happens (simplified to make it easier to follow > - finding > the problem by debugging the Javascript frontend (wrongly) was much more > difficult): > > Connection 1: > > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > > Connection 2: > > ... Wait some time until the user sends a request... > Query: SELECT x FROM a > Result: Nondeterministic number > Query: SELECT x FROM a > Result: Nondeterministic possibly different number (WTF!!!!!) > > This is especially bad if you request extra data from other tables in an extra > query and the join condition suddenly doesn't match (and thus the row isn't > returned!). > > > Better would be if it acted like this: > > Connection 1: > > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > Statement: UPDATE a SET x = x + 1 > > Connection 2: > > ... Wait some time until the user sends a request... > Statement: BEGIN TRANSACTION > Statement: SET TRANSACTION ISOLATION LEVEL SERIALIZABLE > Query: SELECT x FROM a > Result: Some number > Query: SELECT x FROM a > Result: The same number > ... wait however long you want > Query: SELECT x FROM a > Result: The same number > Statement: ROLLBACK TRANSACTION or COMMIT TRANSACTION > > loop > > Statement: BEGIN TRANSACTION > Statement: SET TRANSACTION ISOLATION LEVEL SERIALIZABLE > Query: SELECT x FROM a > Result: Some possibly different number xxx > Query: SELECT x FROM a > Result: The same number xxx as in the previous query > Query: SELECT x FROM a > Result: The same number xxx as in the previous query > ... > >> With that patch, database queries are serialized, which means that if >> query1, query2 and query3 are sent in that order, they will be executed >> in that order, one after the other. > > It depends on what exactly that means. As long as it means that the > entire HTTP request handler is ONE query that is ordered such, that's fine. > > Otherwise not. > > If there are more complicated multiple queries done by the web interface > on behalf of the user because of one HTTP request, we have to make sure > that those queries execute without any interleaving by some writer. > > As a stopgap, this database query serializer should let the user batch > the queries/statements and run each batch in its own transaction. > I think that would be quite okay as a solution, actually, as long as > there are no other shadow clients of the database. > >> Currently, there is only one connection that is shared by the writers >> and readers. Having one 'read connection' and one 'write connection' >> would be possible and make sense if both of them live in a separate >> thread and use the SQLite multithreading feature so that writing and >> reading proceed concurrently. Is that what you mean? > > No.
