Re: [PATCH] Sqlite DB Pool

Alejandro Guerrieri schrieb:
> Any other opinions on this? Shall I commit?

+1 from me, and as no objections were there, please 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
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>>
> 
> 
>