Luís Soares wrote:
> 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
OK
> 4. slow query log tests (with TIMESTAMP changed)
>
Slow query log is not affected by this patch, so I think this test can
be skipped.
> 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?
>
slow query logging used start_utime (in microseconds), which is not
affected by TIMESTAMP, so no need to change that.
> 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==
> >
> >
>
>
>
