bug#32234: [PATCH 2/2] database: Serialize all database accesses in a thread.

["Clément Lassieur" <clement@lassieur.org>]

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 <dannym@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 <clement@lassieur.org> wrote:
>
>> Danny Milosavljevic <dannym@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.