Re: [PATCH] Sqlite DB Pool

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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
> 
> 
>