Quantcast

concurency design/broken design

classic Classic list List threaded Threaded
11 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

concurency design/broken design

Anthony Pankov
Hello, John

I'm really sorry that I still didn't check 8743b7226dc7 patch on issue
"showing  template  sources  to  all".   I  have  own source branch of
roundup-tracker  which  has  not merged for a long time. I have a weak
skill  with  Mercurial so I scare to merge this patch alone. I plan to
do it in a  foreseeable future.

Now I do some exploring in roundup core and have a question.

Each   web   request   processed  in  own  backend instance. Each node
creation (createnode()) call newid() which do the following:
        # get the next ID
        sql = 'select num from ids where name=%s'%self.arg
        self.sql(sql, (classname, ))
        newid = int(self.cursor.fetchone()[0])

        # update the counter
        sql = 'update ids set num=%s where name=%s'%(self.arg, self.arg)
        vals = (int(newid)+1, classname)
        self.sql(sql, vals)

Because  of  forking/threading  for request processing the  same  may be executed by
another process/thread in parallel . Also somewhere later there is a call for commit  "self.conn.commit()".

I  do  not  understand  the  concurrency  aware logic behind it. Is it
there?


--
Best regards,
 Anthony                          mailto:[hidden email]


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Ralf Schlatterbeck-3
On Tue, Apr 18, 2017 at 12:16:53PM +0300, Anthony Pankov wrote:

>
> Each   web   request   processed  in  own  backend instance. Each node
> creation (createnode()) call newid() which do the following:
>         # get the next ID
>         sql = 'select num from ids where name=%s'%self.arg
>         self.sql(sql, (classname, ))
>         newid = int(self.cursor.fetchone()[0])
>
>         # update the counter
>         sql = 'update ids set num=%s where name=%s'%(self.arg, self.arg)
>         vals = (int(newid)+1, classname)
>         self.sql(sql, vals)
>
> Because  of  forking/threading  for request processing the  same  may
> be executed by another process/thread in parallel . Also somewhere
> later there is a call for commit  "self.conn.commit()".
>
> I  do  not  understand  the  concurrency  aware logic behind it. Is it
> there?

The whole code runs in a transaction.
On sqlite (from which you seem to have quoted the code above) the
correctness wrt concurrency is done via a lock maintained by the
database. For other databases the implementation of concurrency control
may be different, for example for PostgreSQL each table has its own
sequencer which is guaranteed to return a new unique ID to each
requesting process.

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

John P. Rouillard
In reply to this post by Anthony Pankov
Hi Anthony:

In message <[hidden email]>,
Anthony Pankov writes:
>I'm really sorry that I still didn't check 8743b7226dc7 patch on issue
>"showing  template  sources  to  all". I  have  own source branch of
>roundup-tracker  which  has  not merged for a long time. I have a weak
>skill  with  Mercurial so I scare to merge this patch alone. I plan to
>do it in a  foreseeable future.

Sounds good. Thanks for keeping it on your todo list.

>Now I do some exploring in roundup core and have a question.
>
>Each   web   request   processed  in  own  backend instance. Each node
>creation (createnode()) call newid() which do the following:
>        # get the next ID
>        sql = 'select num from ids where name=%s'%self.arg
>        self.sql(sql, (classname, ))
>        newid = int(self.cursor.fetchone()[0])
>
>        # update the counter
>        sql = 'update ids set num=%s where name=%s'%(self.arg, self.arg)
>        vals = (int(newid)+1, classname)
>        self.sql(sql, vals)
>
>Because  of  forking/threading  for request processing the  same  may be executed by
>another process/thread in parallel . Also somewhere later there is a call for commit  "self.conn.commit()".
>
>I  do  not  understand  the  concurrency  aware logic behind it. Is it
>there?

IIUC there is a tracker level lock taken on opening the tracker for
the anydb back end. So it serializes all the database activity (one
reason anydb is not recommeded for high volume sites.  For sql based
back ends, I seem to remember there was a table level lock somewhere
upstream of these calls.

One of the others may remember. IIRC we came across it while we were
diagnosing what appeared to be a missing journal entry. This is what
gave rise to the ability to set the database isolation level.

Changing these to an autoincrement field or an atomic increment would
be better but I am not sure it is supported in all backends and I have
no idea how to write the code to convert existing tables.

--
                                -- rouilj
John Rouillard
===========================================================================
My employers don't acknowledge my existence much less my opinions.

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Ralf Schlatterbeck-3
On Tue, Apr 18, 2017 at 08:35:26AM -0400, John P. Rouillard wrote:

>
> IIUC there is a tracker level lock taken on opening the tracker for
> the anydb back end. So it serializes all the database activity (one
> reason anydb is not recommeded for high volume sites.  For sql based
> back ends, I seem to remember there was a table level lock somewhere
> upstream of these calls.
>
> One of the others may remember. IIRC we came across it while we were
> diagnosing what appeared to be a missing journal entry. This is what
> gave rise to the ability to set the database isolation level.

Yes, for anydb there is a locking mechanism.
And for sqlite (from which the example seems to be taken) the
transaction mechanism of the underlying db is used (which for sqlite in
turn again uses a lock :-) so only one process can be in a writing
transaction at a time. Maybe there is an additional lock in sqlite,
though.

> Changing these to an autoincrement field or an atomic increment would
> be better but I am not sure it is supported in all backends and I have
> no idea how to write the code to convert existing tables.

For Postgres (I think mysql doesn't supports it, didn't look how it's
implemented there) we're using this, when viewing the DB with psql look
for "sequence" like in the following example for "category":

database=> \d
 ...
 public | _category_ids                        | sequence | ralf

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Anthony Pankov
In reply to this post by Ralf Schlatterbeck-3
Hello, Ralf.

> On sqlite (from which you seem to have quoted the code above) the
> correctness wrt concurrency is done via a lock maintained by the
> database.

As  I  see  there is not enough magic power in database level to solve
the issue (at least in sqlite case).

I'll try to explain.
Let's *mark* the code.

                    # get the next ID
                    sql = 'select num from ids where name=%s'%self.arg
*SEL*          self.sql(sql, (classname, ))
                    newid = int(self.cursor.fetchone()[0])
                    # update the counter
                    sql = 'update ids set num=%s where name=%s'%(self.arg, self.arg)
                    vals = (int(newid)+1, classname)
*UPD*         self.sql(sql, vals)
   ...
   *COMM*
   self.db.sql_conn.commit()

Imagine two threads/process (1 and 2) which execute the code in the following sequence:
A:  1.SEL, 2.SEL,   ...
B:  1.SEL,  1.UPD, 2.SEL, ...
C:  1.SEL,  1.UPD, 1. COMM, 2.SEL,  ...

In  the  current  code each request instantiate backend interface. Each
backend interface open own database connection.
In  such  a  case SQLite say: "the reader is only able to see complete
committed  transactions  from  the  writer". So, the only troublefree
variant is the C.

We  can  do  the  following:
1) set shared_cache and read_uncommited options to SQLite;
2) share connection between backend instances.

Those  variants are equal and  about  to  add  visibility  of changes on early
stages. Then we make variant B troublefree.


> On Tue, Apr 18, 2017 at 12:16:53PM +0300, Anthony Pankov wrote:
>>
>> Each   web   request   processed  in  own  backend instance. Each node
>> creation (createnode()) call newid() which do the following:
>>         # get the next ID
>>         sql = 'select num from ids where name=%s'%self.arg
>>         self.sql(sql, (classname, ))
>>         newid = int(self.cursor.fetchone()[0])
>>
>>         # update the counter
>>         sql = 'update ids set num=%s where name=%s'%(self.arg, self.arg)
>>         vals = (int(newid)+1, classname)
>>         self.sql(sql, vals)
>>
>> Because  of  forking/threading  for request processing the  same  may
>> be executed by another process/thread in parallel . Also somewhere
>> later there is a call for commit  "self.conn.commit()".
>>
>> I  do  not  understand  the  concurrency  aware logic behind it. Is it
>> there?

> The whole code runs in a transaction.
> On sqlite (from which you seem to have quoted the code above) the
> correctness wrt concurrency is done via a lock maintained by the
> database. For other databases the implementation of concurrency control
> may be different, for example for PostgreSQL each table has its own
> sequencer which is guaranteed to return a new unique ID to each
> requesting process.

> Ralf



--
Best regards,
 Anthony                          mailto:[hidden email]


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Ralf Schlatterbeck-3
On Wed, Apr 19, 2017 at 09:26:58AM +0300, Anthony Pankov wrote:

> Hello, Ralf.
>
> > On sqlite (from which you seem to have quoted the code above) the
> > correctness wrt concurrency is done via a lock maintained by the
> > database.
>
> As  I  see  there is not enough magic power in database level to solve
> the issue (at least in sqlite case).
>
>
> Imagine two threads/process (1 and 2) which execute the code in the following sequence:
> A:  1.SEL, 2.SEL,   ...
> B:  1.SEL,  1.UPD, 2.SEL, ...
> C:  1.SEL,  1.UPD, 1. COMM, 2.SEL,  ...
>
> In  the  current  code each request instantiate backend interface. Each
> backend interface open own database connection.
> In  such  a  case SQLite say: "the reader is only able to see complete
> committed  transactions  from  the  writer". So, the only troublefree
> variant is the C.

Hmm as I understand the sql semantics two overlapping read-modify-write
(select modify update) should not go through because they are not
serializable. If this is different for sqlite we might have a problem.
But did you check, do you get overlapping IDs in roundup? I don't
remember any bug-report on this and I used the sqlite backend for years
in a high-volume setup where I expect we should have seen collisions if
there is a bug.

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Ralf Schlatterbeck-3
On Thu, Apr 20, 2017 at 02:22:36PM +0200, Ralf Schlatterbeck wrote:
> Hmm as I understand the sql semantics two overlapping read-modify-write
> (select modify update) should not go through because they are not
> serializable. If this is different for sqlite we might have a problem.
> But did you check, do you get overlapping IDs in roundup? I don't
> remember any bug-report on this and I used the sqlite backend for years
> in a high-volume setup where I expect we should have seen collisions if
> there is a bug.

Example:

% sqlite3 newdb
...
create table test (id int, value int);
insert into test (id, value) values (1, 1);
begin transaction;
select value from test where id=1;
1
                                % sqlite3 newdb
                                begin transaction;
                                select value from test where id=1;
                                1
update test set value=2 where id=1;
                                update test set value=2 where id=1;
                                Error: database is locked
commit;
Error: database is locked
                                commit;
commit;

Looks like we can only commit one of the transaction with updates.
Also sqlite claims it uses "serializable" isolation by default, so this
should be fine. If you still think we have a problem there, I'd like to
see a demo before investigating further.

Isolation in sqlite:
https://sqlite.org/isolation.html

Thanks
Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Anthony Pankov
In reply to this post by Ralf Schlatterbeck-3
> But did you check, do you get overlapping IDs in roundup?
No, I've never seen it.

But I can breed an example from this theory.
1. Open roundup-admin.
    Run command to create issue:
    create issue title=...
2. Go to web interface of the same tracker.
    Create an issue there. It will block.
3. Return to roundup-admin.
    Run command:
    commit
4. Return to web interface and see an error.

This is an example of B variant.

I   agree  that  practically  it's  not a catastrophic problem.
Nevertheless we can think about some improving :

1.  Since  we  have  already  exploring the core of tracker, should we
propose  the  "shared_cache"  option  for  sqlite.  It  can greatly(?)
improve the performance of very bad logic of web request processing.

2.  Should  we  add "read_uncommitted" option to the sqlite connection. In my
understanding  the  error in example above will gone. But this have a side
effects.


> On Wed, Apr 19, 2017 at 09:26:58AM +0300, Anthony Pankov wrote:
>> Hello, Ralf.
>>
>> > On sqlite (from which you seem to have quoted the code above) the
>> > correctness wrt concurrency is done via a lock maintained by the
>> > database.
>>
>> As  I  see  there is not enough magic power in database level to solve
>> the issue (at least in sqlite case).
>>
>>
>> Imagine two threads/process (1 and 2) which execute the code in the following sequence:
>> A:  1.SEL, 2.SEL,   ...
>> B:  1.SEL,  1.UPD, 2.SEL, ...
>> C:  1.SEL,  1.UPD, 1. COMM, 2.SEL,  ...
>>
>> In  the  current  code each request instantiate backend interface. Each
>> backend interface open own database connection.
>> In  such  a  case SQLite say: "the reader is only able to see complete
>> committed  transactions  from  the  writer". So, the only troublefree
>> variant is the C.

> Hmm as I understand the sql semantics two overlapping read-modify-write
> (select modify update) should not go through because they are not
> serializable. If this is different for sqlite we might have a problem.
> But did you check, do you get overlapping IDs in roundup? I don't
> remember any bug-report on this and I used the sqlite backend for years
> in a high-volume setup where I expect we should have seen collisions if
> there is a bug.

> Ralf



--
Best regards,
 Anthony                          mailto:[hidden email]


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Ralf Schlatterbeck-3
On Fri, Apr 21, 2017 at 10:32:21AM +0300, Anthony Pankov wrote:
> > But did you check, do you get overlapping IDs in roundup?
> No, I've never seen it.
Fine.

> But I can breed an example from this theory.
> 1. Open roundup-admin.
>     Run command to create issue:
>     create issue title=...
> 2. Go to web interface of the same tracker.
>     Create an issue there. It will block.
> 3. Return to roundup-admin.
>     Run command:
>     commit
> 4. Return to web interface and see an error.
>
> This is an example of B variant.

Getting an error there (due to two colliding transactions) is the
expected behaviour and makes sure the data is consistent. Even with a
busy tracker I've never seen such a collision. So "fixing" a problem
that almost never happens isn't something I'd do here.

> 1.  Since  we  have  already  exploring the core of tracker, should we
> propose  the  "shared_cache"  option  for  sqlite.  It  can greatly(?)
> improve the performance of very bad logic of web request processing.
>
> 2.  Should  we  add "read_uncommitted" option to the sqlite connection. In my
> understanding  the  error in example above will gone. But this have a side
> effects.

This will kill the serializable properties of sqlite transactions. This
property keeps the data consistent in the first place. So, no, this will
make things worse (unless you employ a non-standard mechanism to
synchronize parallel processes which should be left to tested code of
the database).

If you have bad performance with sqlite use a different database. In
some busy trackers I'm running I'm using postgres which I can highly
recommend. Optimizing a database backend for performance when other
backends already have far superior performance is a waste of time.
Sqlite is by design a single-threaded database and uses locking for
critical sections. Postgres is designed for parallel execution from the
ground up.

Ralf
--
Dr. Ralf Schlatterbeck                  Tel:   +43/2243/26465-16
Open Source Consulting                  www:   http://www.runtux.com
Reichergasse 131, A-3411 Weidling       email: [hidden email]

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Anthony Pankov
> Getting an error there (due to two colliding transactions) is the
> expected behaviour and makes sure the data is consistent. Even with a
> busy tracker I've never seen such a collision. So "fixing" a problem
> that almost never happens isn't something I'd do here.

This is unusual approach to the concurrency design.

>> 1.  Since  we  have  already  exploring the core of tracker, should we
>> propose  the  "shared_cache"  option  for  sqlite.  It  can greatly(?)
>> improve the performance of very bad logic of web request processing.
>>
>> 2.  Should  we  add "read_uncommitted" option to the sqlite connection. In my
>> understanding  the  error in example above will gone. But this have a side
>> effects.

> This will kill the serializable properties of sqlite transactions.

What is your opinion based on?

--
Best regards,
 Anthony                          mailto:[hidden email]


------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: concurency design/broken design

Cédric Krier
On 2017-04-21 11:45, Anthony Pankov wrote:
> > Getting an error there (due to two colliding transactions) is the
> > expected behaviour and makes sure the data is consistent. Even with a
> > busy tracker I've never seen such a collision. So "fixing" a problem
> > that almost never happens isn't something I'd do here.
>
> This is unusual approach to the concurrency design.

Indeed such operational error may always happen when using at least
"Repeatable Read" isolation level [1]. Indeed it would be good to retry
the action in case OperationError is raised, this would prevent showing
error page to the user.


[1] https://www.postgresql.org/docs/9.1/static/transaction-iso.html#XACT-REPEATABLE-READ

--
Cédric Krier - B2CK SPRL
Email/Jabber: [hidden email]
Tel: +32 472 54 46 59
Website: http://www.b2ck.com/

------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
_______________________________________________
Roundup-devel mailing list
[hidden email]
https://lists.sourceforge.net/lists/listinfo/roundup-devel

signature.asc (352 bytes) Download Attachment
Loading...