Re: [PATCH] Sqlite DB Pool

Any other opinions on this? Shall I commit?

Regards,
--
Alejandro Guerrieri
[email protected]


El 20/11/2008, a las 02:51 p.m., Alexander Malysh escribió:

> Alejandro Guerrieri schrieb:
>> Alex,
>> SQLite doesn't wait for the lock to free, it returns with a BUSY  
>> error inmediately. The query will return BUSY without waiting for  
>> the lock even when the process is writing to another table on the  
>> same DB.
>
> ahh now it's clear to me and I'm +1 to commit your patch.
>
>> From my tests, I can confirm that this is what it happens.
>> I understand your point about "DB agnosticism", but as it is now  
>> sqlite is not a viable alternative for sqlbox at least. The same  
>> probably applies to DLR's, if more than a thread is accessing the  
>> DB at the same time.
>> As I've said before, the PHP client implementation set this value  
>> to 60 seconds (which is a little high imho), DBD::SQlite sets it to  
>> 30 seconds and most other client libraries I've found provides a  
>> mean to set this, because its default value is non-practical for  
>> most cases.
>> Are you fine with adding a fixed value instead of being user- 
>> selectable? This wouldn't change the user interface, only the  
>> internals when dealing with sqlite DB's.
>> Regards,
>> Alex
>> El 20/11/2008, a las 12:56 p.m., Alexander Malysh escribió:
>>> Hi Alex,
>>>
>>> thanks for explaining of the sqlite internals but it didn't answer  
>>> my question: why do we need in dbpool to care about some internals  
>>> of any DB?
>>>
>>> For kannel it's not a point when DB blocks and how oft and how  
>>> long... Kannel just waits for a DB to be unlocked and do it so  
>>> long as DB think is needed.
>>>
>>> Thanks,
>>> Alex
>>>
>>> Alejandro Guerrieri schrieb:
>>>> Alex,
>>>> Sqlite makes a lock on the _whole DB file_ each time it access  
>>>> it. That means that if an external application is doing an INSERT  
>>>> on send_sms, it will lock the WHOLE FILE, so sqlbox won't get a  
>>>> LOCK to DELETE on send_sms nor to INSERT on sent_sms.
>>>> If the DB is to sustain moderate load, the default won't cut it.  
>>>> I've experienced the error when trying to enqueue messages from  
>>>> Sqlite's command line utility while running sqlbox. I'm talking  
>>>> about sending a _single_ message on an idle sqlbox process.
>>>> Using the default "busy_timeout" is too low for most practical  
>>>> uses IMHO.
>>>> For example, the PHP driver sets it to 60 seconds.
>>>> If not allowed to change it on the DBPool, it should be set to  
>>>> some more realistic value at least, or maybe even set a default  
>>>> and allow the client to change it (I can modify the patch to  
>>>> accomodate either case of course).
>>>> Regards,
>>>> Alejandro Guerrieri
>>>> El 20/11/2008, a las 07:40 a.m., Alexander Malysh escribió:
>>>>> Hi Alex,
>>>>>
>>>>> why should it be necessary to define lock_timeout? I ask because  
>>>>> we don't allow it for any DBs from our dbpool why should we make  
>>>>> exception for sqlite?
>>>>>
>>>>> Thanks,
>>>>> Alex
>>>>>
>>>>> Alejandro Guerrieri schrieb:
>>>>>> Alex? Did you see this? I've already commited the updates on  
>>>>>> sqlbox and this would definitely improve sqlite2/3 usability.
>>>>>> Regards,
>>>>>> Alejandro
>>>>>> El 13/11/2008, a las 01:20 a.m., Alejandro Guerrieri escribió:
>>>>>>> Did anyone reviewed this again? I think it's now fixed.
>>>>>>>
>>>>>>> I'm about to commit a big update on sqlbox and it would be  
>>>>>>> great to be able to have sqlite2 and sqlite3 behaving properly.
>>>>>>>
>>>>>>> Regards,
>>>>>>>
>>>>>>> Alejandro Guerrieri
>>>>>>>
>>>>>>> El 08/11/2008, a las 12:17 p.m., Alejandro Guerrieri escribió:
>>>>>>>
>>>>>>>> Ups, > 0 would be better right?
>>>>>>>>
>>>>>>>> Changed the datatype to int because that's what the  
>>>>>>>> sqlite_busy_timeout() expects.
>>>>>>>>
>>>>>>>> [Too little coffee that night] ;)
>>>>>>>>
>>>>>>>> Fixed on the code plese re-download.
>>>>>>>>
>>>>>>>> One last note: the value must be properly initialized,  
>>>>>>>> otherwise the struct default could be anything. I've checked  
>>>>>>>> the Kannel code and since there's no dlr_sqlite.c nor any  
>>>>>>>> other use for this (yet), no further patching is necessary.
>>>>>>>>
>>>>>>>> Regards,
>>>>>>>>
>>>>>>>> Alejandro Guerrieri
>>>>>>>>
>>>>>>>> El 07/11/2008, a las 01:57 p.m., Alexander Malysh escribió:
>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> hmm, are you sure ? ;)
>>>>>>>>>
>>>>>>>>> +    if (conf->lock_timeout != NULL) {
>>>>>>>>>
>>>>>>>>> but lock_timeout defined as long...
>>>>>>>>>
>>>>>>>>> Thanks,
>>>>>>>>> Alex
>>>>>>>>>
>>>>>>>>> Alejandro Guerrieri schrieb:
>>>>>>>>>> Hi,
>>>>>>>>>> I've made this small patch that fixes a couple of things on  
>>>>>>>>>> the Sqlite implementation:
>>>>>>>>>> * Sqlite2's [sqlite-connection] group was missing from  
>>>>>>>>>> cfg.def, though dbpool_sqlite.c and all the hooks were in  
>>>>>>>>>> place. This patch adds the missing section.
>>>>>>>>>> * This patch adds a "lock-timeout" configuration option to  
>>>>>>>>>> both sqlite connection groups, where a value in  
>>>>>>>>>> milliseconds can be specified. That value controls the time  
>>>>>>>>>> sqlite waits before throwing a lock ("busy") error. This  
>>>>>>>>>> could be an issue because sqlite locks at the database  
>>>>>>>>>> level (yuk!), and this can surely be a problem under  
>>>>>>>>>> moderate load (I wouldn't recommend sqlite for heavy  
>>>>>>>>>> traffic at all). If not set the default behaviour is  
>>>>>>>>>> maintained, so nothing's broken ;)
>>>>>>>>>> Please check the post here:
>>>>>>>>>> http://www.blogalex.com/archives/50
>>>>>>>>>> Direct download:
>>>>>>>>>> http://www.blogalex.com/wp-content/uploads/2008/11/kannel-sqlite-pool-patches.diff 
>>>>>>>>>>  BTW, my long-term goal is to have on Sqlbox the same DB  
>>>>>>>>>> capabilities as Kannel have (Sqlbox only supports MySql and  
>>>>>>>>>> Postgres and _maybe_ MS SQL). I'm making good progress with  
>>>>>>>>>> sqlite so far, and this patches aim to complete the Kannel  
>>>>>>>>>> support to make it practical to use the DB pool with Sqlbox.
>>>>>>>>>> MS SQL by Ct-Lib is supposed to be working, though is not  
>>>>>>>>>> supported at Kannel afaik (I'll try to port the code to  
>>>>>>>>>> Kannel so it can be linked from gwlib as the rest does when  
>>>>>>>>>> I have some spare time).
>>>>>>>>>> Regards,
>>>>>>>>>> Alejandro Guerrieri
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>
>>>
>
>