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

[Danny Milosavljevic <dannym@scratchpost.org>]

[-- Attachment #1: Type: text/plain, Size: 4803 bytes --]

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.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]