review all sql to ensure that it works with concurrent requests

Details

Description

this is just a reminder ticket .. it is important that all SQL code in D2 takes concurrent requests into account. this means simply fetching data and then writing data back to the RDBMS needs to take into account that there could be concurrent requests that change the fetched data. this means employing necessary explicit locking or optimistic locking etc.

Activity

I think we dont use a read - update - write approach that is supposed to be atomic anywhere. Its obvious that this is fragile. Its like the select max() + increment in memory + write record variant for generating primary keys which is obviously a bad idea (at least it would need SERIALIZABLE transaction isolation I think).

Roman S. Borschel
added a comment - 12/Mar/10 4:43 AM - edited I think we dont use a read - update - write approach that is supposed to be atomic anywhere. Its obvious that this is fragile. Its like the select max() + increment in memory + write record variant for generating primary keys which is obviously a bad idea (at least it would need SERIALIZABLE transaction isolation I think).
(Obviously, any object retrieval -> modification -> persistence is a read-update-write but thats expectedly not "concurrency safe". Thats what optimistic or pessimistic (not yet implemented) locking is for.)

This is looking quite promising. I guess it would be wise to give each model class the choice if it wants to cause a lock or not, because especially in high concurrency scenarios locking a table can of course have some drastic performance effects. Actually if I for example have a sortable behavior and I am not messing with the sorting at all, I do not need to lock the table either. So probably the best approach would be that if a model method is called that requires locking. Each model could register an onFlush() event (theoretically it could try and be smart and only register the event when it knows it has something to do) and then given the value of the flag tell the lock class to issue a lock for the given underlying table.

BTW: as for innodb .. like i said some RDBMS have this behavior .. but its nothing we can rely on at all ..

Lukas Kahwe
added a comment - 13/Mar/10 7:07 AM This is looking quite promising. I guess it would be wise to give each model class the choice if it wants to cause a lock or not, because especially in high concurrency scenarios locking a table can of course have some drastic performance effects. Actually if I for example have a sortable behavior and I am not messing with the sorting at all, I do not need to lock the table either. So probably the best approach would be that if a model method is called that requires locking. Each model could register an onFlush() event (theoretically it could try and be smart and only register the event when it knows it has something to do) and then given the value of the flag tell the lock class to issue a lock for the given underlying table.
BTW: as for innodb .. like i said some RDBMS have this behavior .. but its nothing we can rely on at all ..

yeah of course, this code locks all the time, however some kind of configuration to check for models that need full table locks is probably necessary, but as you can see from the code, easy to implement.

My idea would be to add a "TableLockRequired" marker interface and in the loops check if any dirty model implements this marker interface.

Benjamin Eberlei
added a comment - 14/Mar/10 1:11 PM yeah of course, this code locks all the time, however some kind of configuration to check for models that need full table locks is probably necessary, but as you can see from the code, easy to implement.
My idea would be to add a "TableLockRequired" marker interface and in the loops check if any dirty model implements this marker interface.

Just as a reminder, I think if some code attempts to lock and the current RDBMS/Engine doesn't allow the requested lock, D2 should (make it an option maybe, but default true imo) throw an exception saying locking doesn't work with the current engine. Silently doing nothing is good in production to avoid pages exploding, but in development I want to know when I lock if it's not really locking in the background. And if I can know that without reading all detailed specs about whatever engine is in use that's even better.

Jordi Boggiano
added a comment - 14/Mar/10 1:23 PM Just as a reminder, I think if some code attempts to lock and the current RDBMS/Engine doesn't allow the requested lock, D2 should (make it an option maybe, but default true imo) throw an exception saying locking doesn't work with the current engine. Silently doing nothing is good in production to avoid pages exploding, but in development I want to know when I lock if it's not really locking in the background. And if I can know that without reading all detailed specs about whatever engine is in use that's even better.

Synchronizing schedule with DDC-178. This issue is considered resolved when the improved locking support from DDC-178 has been implemented. As Benjamin demonstrated more custom (and generic, reusable) locking strategies are possible through the use of the event system.

Roman S. Borschel
added a comment - 15/Mar/10 6:28 PM Synchronizing schedule with DDC-178 . This issue is considered resolved when the improved locking support from DDC-178 has been implemented. As Benjamin demonstrated more custom (and generic, reusable) locking strategies are possible through the use of the event system.