List:Commits« Previous MessageNext Message »
From:Luís Soares Date:May 4 2010 1:16pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3453)
Bug#52704
View as plain text  
Hi Zhenxing,

  Nice Work. 
  Please find my review comments below.

STATUS
------
 
  Not approved. (missing some test cases and there is also a
                 request for feedback).

REQUIRED CHANGES
----------------
 
  RC1. Please add a test case, covering several scenarios:

       1. regular insert/updates
       2. insert delayed
       3. inserts in events 
       4. slow query log tests (with TIMESTAMP changed)

REQUESTS
--------
 
  R1. This is a request for feedback. 

      What are the implications for slow query logging? Should
      this new real_start_time be now used instead of start_time?

SUGGESTIONS
-----------
 n/a

DETAILS 
-------
 n/a


On Sat, 2010-05-01 at 12:03 +0000, He Zhenxing wrote:
> #At file:///media/sda3/work/mysql/bzrwork/b52704/5.1-bugteam/ based on
> revid:svoj@stripped
> 
>  3453 He Zhenxing	2010-05-01
>       Bug#52704 exec_time=4294967295
>       
>       thd->start_time was used to calculate the exec_time of the query,
>       since thd->start_time can be affected by TIMESTAMP session variable,
>       this would result in a very big value when TIMESTAMP is set to a
>       future time before execution a query.
>       
>       Fixed by adding a new varialbe thd->real_start_time, which will
>       not be affected by TIMESTAMP, and use this to calculate the
>       execution time of the query.
>      @ sql/log_event.cc
>         use real_start_time to calculate the execution time of the query, which is
> not affected by the TIMESTAMP session variable
>      @ sql/sql_class.h
>         add real_start_time to record the real system start time of the query, which
> is not affected by the TIMESTAMP session variable
> 
>     modified:
>       sql/log_event.cc
>       sql/sql_class.h
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc	2010-03-28 11:57:33 +0000
> +++ b/sql/log_event.cc	2010-05-01 12:03:34 +0000
> @@ -2394,7 +2394,8 @@ Query_log_event::Query_log_event(THD* th
>    error_code= errcode;
>  
>    time(&end_time);
> -  exec_time = (ulong) (end_time  - thd_arg->start_time);
> +  DBUG_ASSERT(end_time >= thd_arg->real_start_time);
> +  exec_time = (ulong) (end_time  - thd_arg->real_start_time);
>    /**
>      @todo this means that if we have no catalog, then it is replicated
>      as an existing catalog of length zero. is that safe? /sven
> @@ -4241,7 +4242,8 @@ Load_log_event::Load_log_event(THD *thd_
>  {
>    time_t end_time;
>    time(&end_time);
> -  exec_time = (ulong) (end_time  - thd_arg->start_time);
> +  DBUG_ASSERT(end_time >= thd_arg->real_start_time);
> +  exec_time = (ulong) (end_time  - thd_arg->real_start_time);
>    /* db can never be a zero pointer in 4.0 */
>    db_len = (uint32) strlen(db);
>    table_name_len = (uint32) strlen(table_name);
> 
> === modified file 'sql/sql_class.h'
> --- a/sql/sql_class.h	2010-04-14 09:53:59 +0000
> +++ b/sql/sql_class.h	2010-05-01 12:03:34 +0000
> @@ -1414,6 +1414,9 @@ public:
>    /* remote (peer) port */
>    uint16 peer_port;
>    time_t     start_time, user_time;
> +  time_t     real_start_time;        /* The real start time of the
> +                                      * query, not affected by
> +                                      * TIMESTAMP session variable */
>    // track down slow pthread_create
>    ulonglong  prior_thr_create_utime, thr_create_utime;
>    ulonglong  start_utime, utime_after_lock;
> @@ -2008,19 +2011,14 @@ public:
>    inline time_t query_start() { query_start_used=1; return start_time; }
>    inline void set_time()
>    {
> -    if (user_time)
> -    {
> -      start_time= user_time;
> -      start_utime= utime_after_lock= my_micro_time();
> -    }
> -    else
> -      start_utime= utime_after_lock= my_micro_time_and_time(&start_time);
> +    start_utime= utime_after_lock= my_micro_time_and_time(&real_start_time);
> +    start_time = user_time ? user_time : real_start_time;
>    }
> -  inline void	set_current_time()    { start_time= my_time(MY_WME); }
> +  inline void	set_current_time() { real_start_time= start_time= my_time(MY_WME); }
>    inline void	set_time(time_t t)
>    {
>      start_time= user_time= t;
> -    start_utime= utime_after_lock= my_micro_time();
> +    set_time();
>    }
>    void set_time_after_lock()  { utime_after_lock= my_micro_time(); }
>    ulonglong current_utime()  { return my_micro_time(); }
> 
> text/bzr-bundle type attachment
> (bzr/zhenxing.he@stripped)
> # Bazaar merge directive format 2 (Bazaar 0.90)
> # revision_id: zhenxing.he@stripped
> # target_branch: file:///media/sda3/work/mysql/bzrwork/b52704/5.1-\
> #   bugteam/
> # testament_sha1: fe802bc5affef390af9d6cb3c9a202abe51c0cd9
> # timestamp: 2010-05-01 20:03:47 +0800
> # base_revision_id: svoj@stripped
> # 
> # Begin bundle
> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWW1UD+oAAwbfgGQwWXf///fi
> XrC////6YAcLnznd2ihYrBR0pd3AKuElAmpT00T8jVN5Ub2inqntTap5NR6hkyDIYjQGgSRImU/T
> RqeppRgTNT0gZDCMEwEaAGg4aaZGIwmmAhgE0wjBMTIaZGhoBIiImjUxGTTU0eoeppoxGT1BkAAN
> MjQOGmmRiMJpgIYBNMIwTEyGmRoaASSEwhNGIAmjQFPTFPUzKZkEAAPTUnmR0Z6Km3Uy/zE47e/q
> PjZblyS/vU0cPm3k1MCpl/il5HwQEYR6oxhpsbOrqvo+nI2uknX5o5AMBxOZ6F+8ZKnfWgzDbBsP
> z9kZMdcJmgzcVq0043vNTNIm22uCaS8iWQ2uLt0HEat2pXLUZppE01itU1uIvZYz8aCdJP/Y+Owo
> QOS7lLUf3UOwYiwZO/1xfI/hi4LOf+szqgsPcQJvvGHBW4zmo0UH2xiM7xrNlA9M81qWunTjZf+3
> O1YxZk5DJDxmLm66jQA7DDjeAPutUefhBp2jFvxIEEfK7cyPnhg1NcXrXtVa8qwzoVgGomoCzlxm
> wkoJlK8gp7e1bCaPUJaAwQM2/8fIsecVY9mpQlp2rklLep04BbqSslCoKRbYErqHnGUFUhBJyJjA
> Q9hoNjixwmUKGy4wA5q9t4GVvHkqqYBVRcB5LLhMmBtTLs+NrVP1FkXG1EyuUq1a11Y0Ayx9uChG
> TxXlWFief6TJDkqLC9pyrEqhSQTaDnqweTxI0fle/G+lIykiPu8DNYrgOcChOwzWq63MKUhw5gIA
> XLaaRxgMOrW0eXk+0CBtVDXX+U5Ro0cuNAIWCK3B1ie2tUsNCxhUSIjSN6M7kxEyHeYNKca8uo99
> AN0sPlfNWb7z0JlWJZlUTCqqgrYwYqNvGudTnpmNMOS0y2p9RIyECQ+Q4ySClxYch9+nRA9AKiMr
> WrdWwnOc4s8D5SroTN/c4NQ8oWjzrSL7tFW/SXw1bDVJ9VRdDMZByYkM65mzJWw85eQ9kTMWTKy5
> LPeYO0hM7jGBsZKmJzMkYZITC4q1e6RVFlFbfzib5kbftKbIS4GshyVKEweR4XcR5XK3kN8oyOD0
> Lh1zSJdlIxFZ5AmebxBAHOLsIzDTb+p0Z7jpr1ECjrMeIX76xdIvjgFIiVDi/ZAUkvcUsh+j+Ymy
> HOjc1kVDjLZTI3NU6vhZRrAkW3GDYVkmJalljET0RjCTjOMxCnpW/y+kDyD4Eaq8ysHI14hgipCp
> Wm+r9AX1RpTN5eWUCAWIoIPPWj7kyicecobTzLRx5nFAzFbP1J3fMLdS3alVEVvakNrDUBHeQtPa
> bgznF8C0PFGA8s+V3wMVBsJO5+CHViVlzwmnI1qLlBksIcr46FifE8zW4rREHEqVBCBUBMHowCxZ
> mX0Tjcp3LTWXtKDs1kk7jqSiBb9b/CptsMb32gubW6dZe8c6WJgG4kTNLLrtZyKy9BVl6FUXIih0
> fmXIUvUHTQ5PTMY4tKXVP5mmcfEO1EUyYTxG7CJauIUtot5jiEQaxwcEOQauUXC1GybcEbkugxHj
> cRhxDCviMC0ClXorPqa856SlaVQSqNEFJW8lK/ZzlDM4ODzeDCHmoDiT+JFpJkn5FpP+CqhUqjXu
> XYDfZwM6fzCbvAggPNpINTQ9dyDn7E9RMy2qxA442PA3KwU6oTaZalKXAvQIzGxTaLREagEcxd3I
> wx59NrILZ93XJ9ljDAnLQcEGPrUXd4GgMy1rlNwDUNaJ9RvCwGQ5IijpTgb6cMhjEwYiZabovjvc
> s6+jzbem7KMs9y2LbWJQrU4gHBgzGGiBqrHWoRkRiri1UWK1dsvy3PF0rBJgKhpWiyzYnCMARFts
> BAxDnEsVKrXl14T5rKbAMiRRhXpycItpRDZye1siHI5kbklWAdcCuSVaI1h7XreuAafZRBl6FmXD
> hxipFUFvMnHAnNMYT6lwcdmKeZ1egrUVaSWYWnhtFgjcxhCIdJ0Mf0/0Y6onGwn6+hxsZPggulcr
> lgdZp4R0CxrEFZRkMwmPdzBBnbENVQZnh0TRwvRH6/vBXLKtGSwGAgWhvFyrDw7lsa1zndow5yfo
> ReBpTYmG/KMwD2fy2Ij6iTupNkY4gNC7FZZdcGPAyFEdwQiQgdoRQ57kixIKlVLciGzryBZMccTA
> De8x3mRBVttNAd5twoV0IxIyisYd2yQZ59rQL48nTist3gXXJklnjsvDBnqekeLWiid0qtX98UFa
> ByqeHPWoDBplEtMrCI5Veqozh2jMQQlwJxmcyGSCj61lbuVu+tPJqMAYRzVSVJ/xdyRThQkG1UD+
> oA==
> 
> 


Thread
bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3453) Bug#52704He Zhenxing1 May
  • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3453)Bug#52704Luís Soares4 May
    • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3453)Bug#52704He Zhenxing12 May
  • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3453)Bug#52704Libing Song12 May
    • Re: bzr commit into mysql-5.1-bugteam branch (zhenxing.he:3453)Bug#52704Libing Song13 May