Re: [Sqlgrey-users] PATCH: last_dbclean

[Dan F. <da...@ha...>]

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');

.......

# updateconfig() returns affected_rows
if ($self->updateconfig('last_dbclean',time(),$self->{sqlgrey}{last_dbclean})) {
     ...cut...
} else {
    #If affected_rows == 0, then someone already cleaned db
    $self->{sqlgrey}{last_dbclean} = 0; #make sqlgrey reload time from db on next pass
}



- Dan



Lionel Bouton wrote:
> Hi,
>
> I'm back from my winter holidays. I initially liked your patch to
> updateconfig because it seemed cleaner than what we discussed (at least
> less complex) but then I realized there are 2 bugs. update_config
> doesn't try to check for an $old_value if it is 0 which makes it
> impossible to check for a successful update from a value expected to be
> 0. This is a corner case, but you can avoid it by using
> (defined $old_value) ? ...
> instead of
> ($old_value) ? ...
> This is only a design problem with no actual consequence now but it's
> safer to avoid any complication... Anyway the second bug is quite
> serious and you'll have to remove the patch to update_config unless you
> find another need for it...
>
> Given that you only check for equality and not the timestamp being
> expired at most only one SQLgrey instance will ever update the value and
> then clean up (looking at the details, this will be the last instance
> being launched). 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), this could be fixed, but you can't avoid
> the "cleanup" instance shuting down stops cleanup if you check for equality.
>
> Conclusion : you can't use the config table, given that it only stores
> varchars (you can't compare timestamps stored as varchars). You need to
> create another table with only one stored value in a timestamp column
> and check for expiration not equality.
>
> Lionel.
>
>