List:Commits« Previous MessageNext Message »
From:Dmitry Lenev Date:January 12 2011 11:24am
Subject:Re: bzr commit into mysql-5.5 branch (davi:3236) Bug#42054
View as plain text  
Hello Davi!

I only have a few cosmetic comments about your patch:

* Davi Arnaut <davi.arnaut@stripped> [11/01/11 19:34]:
> # At a local mysql-5.5 repository of davi
> 
>  3236 Davi Arnaut	2011-01-11
>       Bug#42054: SELECT CURDATE() is returning bad value
>       
>       The problem from a user point of view was that on Solaris the
>       time related functions (e.g. NOW(), SYSDATE(), etc) would always
>       return a fixed time.
>       
>       This bug was happening due to a logic in the time retrieving
>       wrapper function which would only call the time() function every
>       half second. This interval between calls would be calculated
>       using the gethrtime() and the logic relied on the fact that time
>       returned by it is monotonic.
>       
>       Unfortunately, due to bugs in the gethrtime() implementation,
>       there are some cases where the time returned by it can drift
>       (See Solaris bug id 6600939), potentially causing the interval
>       calculation logic to fail.
>       
>       Since newer versions of Solaris (10+) have alleviated the
>       performance degradation associated with time(2), the solution is
>       to simply directly rely on time() at each invocation.
>       
>       This simplification has an upside that it allows us to eliminate
>       a lock which was used to control access to the variables used
>       to track the half second interval, thus improving the overall
>       scalability of timekeeping related functions (e.g. NOW()).
>       
>       In spite of the the aforementioned problem, gethrtime() is still
>       used in a few places where we need a high resolution but under
>       which drifts don't pose a problem.

Maybe it makes sense to mention more explicitly:

1) That my_time() and my_micro_time_and_time() no longer use gethrtime()
   because of problem with this function mentioned above. Instead they
   use time() and gettimeofdate() correspondingly.

2) That we run some benchmarks and that their results has not shown
   significant degradation. On the contrary they show that situation
   has improved for cases in which many connections are used.

3) my_micro_time() was changed not to use gethrtime() as well, to have
   a common time source with my_micro_time_and_time(). We don't expect
   significant performance regressions from this change since a) this
   function is used only a few times during statement execution b) on
   Solaris (AFAIU this is the only platform with gethrtime() which
   we support for 5.5) gettimeofday() also has a good performance.

?

>      @ mysys/my_getsystime.c
>         Use time() even if gethrtime() is available. Remove logic which
>         relied on gethrtime() to only call time() every half second.
>         Since gethrtime() is not used anymore, also remove it from
>         my_micro_time() to keep a common time source.
>         
>         Also, function comments are cleaned up (fixed typos and wrong
>         information) and converted to doxygen.


...

> === modified file 'mysys/my_getsystime.c'
> --- a/mysys/my_getsystime.c	2011-01-11 13:53:50 +0000
> +++ b/mysys/my_getsystime.c	2011-01-11 16:32:04 +0000

...

> -/*
> +/**
>    Return time in seconds and timer in microseconds (not different start!)
>  
> -  SYNOPSIS
> -    my_micro_time_and_time()
> -    time_arg		Will be set to seconds since epoch (00:00:00 UTC,
> -                        January 1, 1970)
> -
> -  NOTES
> -    This function is to be useful when we need both the time and microtime.
> -    For example in MySQL this is used to get the query time start of a query
> -    and to measure the time of a query (for the slow query log)
> -
> -  IMPLEMENTATION
> -    Value of time is as in time() call.
> -    Value of microtime is same as my_micro_time(), which may be totally
> -    unrealated to time()
> +  @param  time_arg  Will be set to seconds since epoch.
>  
> -  RETURN
> -    Value in microseconds from some undefined point in time
> -*/
> +  @remark This function is to be useful when we need both the time and
> +          microtime. For example in MySQL this is used to get the query
> +          time start of a query and to measure the time of a query (for
> +          the slow query log)
>  
> -#define DELTA_FOR_SECONDS 500000000LL  /* Half a second */
> +  @retval Value in microseconds from some undefined point in time.
> +*/
>  

IMO it still makes sense to mention in this comment that value returned
by this function is same as returned by my_micro_time(), so two this
functions can be used together. What do you think?


I think it is OK to push this patch after considering
the above comments.

-- 
Dmitry Lenev, Software Developer
Oracle Development SPB/MySQL, www.mysql.com

Are you MySQL certified?  http://www.mysql.com/certification
Thread
bzr commit into mysql-5.5 branch (davi:3236) Bug#42054Davi Arnaut11 Jan
Re: bzr commit into mysql-5.5 branch (davi:3236) Bug#42054Dmitry Lenev12 Jan