Re: [Sqlgrey-users] PATCH: last_dbclean

[David L. <da...@la...>]

Lionel Bouton wrote:
> Dan Faerch wrote the following on 30.01.2007 00:02 :
>> Welcome back. Hope you enjoyed you trip :)
>>
>>
>> Bug 1:
>>   
>>> (defined $old_value) ? ...
>>> instead of
>>> ($old_value) ? .
>>>     
>> Correct. Nicely spotted.  
>>
>> Bug 2:
>>   
>>> If this instance is shut down no other instance will
>>> try to cleanup. You could even end with no cleanup occuring if the two
>>> successive calls to time() give back two different values (your local
>>> copy won't match the database), 
>>>     
>> If i understand you correctly, i disagree with your analysis. I believe the patch handles this perfectly.
>>
>> The patch makes it like this: 
>> - If an UPDATE fails (because of timestamp mismatch) then $last_dbclean = 0;
>> And just before the cleanup time comparision, there is an:
>> - If $last_dbclean == 0 then reload value from db.
>>
>> Which basically means:
>> if UPDATE affected_rows = 0 then SELECT last_dbclean from config.
>>
>> Thus, if db_cleantime = 30mins, all sqlgrey's, except the one who did the cleaning, reloads the timestamp from DB every 30mins. If no-one did the cleaning, everyone will reload and the cycle will continue.
>>
>>
>> Relevant code:
>>
>> #If last_dbclean == 0, reload value from DB
>> if ($self->{sqlgrey}{last_dbclean} == 0) {
>>     $self->{sqlgrey}{last_dbclean} = $self->getconfig('last_dbclean');
>>   
> 
> I see, I misread the patch and thought this code was in insertconfig. I
> just looked at the whole patched script and things makes more sense to
> me now. So the only things left are the (defined $old_value) and
> the 2 time() calls that could introduce an inefficiency.

I wouldn't be worried about inefficiency, it's a cheap system call. I 
would be far more worried about internal inconsistencies. You'll wind up 
with cases where time() rolls over to the next second in the two places 
where it's called and depending on what the rest of the code does, this 
can lead to bugs that are very hard to track down. And if the 
update_config() takes a significant amount of time, the two values could 
be very far apart.

Taking a snapshot of of time() and holding it in a variable is the right 
thing to do.

> In fact I don't really like last_dbclean == 0 being a special case. I'm
> patching the code to use undef, it makes things clearer to me.
> 
> The patch is attached and commited to CVS. Not yet tested (I've only
> spent some time on it before going to work). Please tell me what you
> think of it.

>      # Is it time for cleanups ?
> -    if (time() > ($self->{sqlgrey}{last_dbclean} + $self->{sqlgrey}{db_cleandelay})) {
> -
> +    my $current_time = time();
> +    if ($current_time() > ($self->{sqlgrey}{last_dbclean} + $self->{sqlgrey}{db_cleandelay})) {
            ^^^^^^^^^^^^^^^

That won't compile as is. I think you mean $current_time.

Later,
David

>              # updateconfig() returns affected_rows
> -            if ($self->updateconfig('last_dbclean',time(),$self->{sqlgrey}{last_dbclean})) {
> +            if ($self->updateconfig('last_dbclean',$current_time,$self->{sqlgrey}{last_dbclean})) {
>                    # If affected_rows > 0, its my job to clean the db
> -                  $self->{sqlgrey}{last_dbclean} = time();
> +                  $self->{sqlgrey}{last_dbclean} = $current_time;