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==
>
>
