Re: bzr commit into mysql-5.0-bugteam branch (zhenxing.he:2814) Bug#45520

[He Zhenxing]

Hi Andrei,

Thank you for the review!

Andrei Elkin wrote:
> Zhen Xing, hello.
> 
> 
> 
> > #At file:///media/sdb2/hezx/work/mysql/bzrwork/b45520/5.0-bugteam/ based on
> revid:joro@stripped
> >
> >  2814 He Zhenxing	2009-09-28
> >       BUG#45520 rpl_killed_ddl fails sporadically in pb2
> >       
> >       There are three issues that caused rpl_killed_ddl fails sporadically
> >       in pb2:
> >       
> >        1) error_code can be ER_QUERY_INTERRUPTED/ER_SERVER_SHUTDOWN when
> >           killed_status is NOT_KILLED
> >        2) DATABASE d2 might do exist because the statement to CREATE or
> >       ALTER it was killed
> >        3) because of bug 43353, kill the query that do DROP FUNCTION or
> >           DROP PROCEDURE can result in SP not found
> >       
> 
> I had to dig it out that at least a part of the problem belongs to BUG#37145 fixes,
> and which (http://lists.mysql.com/commits/71714) you are backporting.
> I suggest always refer to a related work in the cset comments.
> 

OK, will add the reference

> 
> >       This patch fixed all above issues by:
> >        1) Make sure error_code is not set to ER_SERVER_SHUTDOWN or 
> >           ER_QUERY_INTTERUPTED if killed_status is NOT KILLED
> 
> About this backporting, first I don't remember if I saw the referred 71714 at time of
> reviewing bug#37145. If I really saw I must have asked questions.
> 

It was added as a postfix

> killed_status as NOT KILLED provided to Query_log_event constructor
> in many cases (for instance in mysql_truncate) in combined with prior (the optimal
> way) or posterior 
> 
>    thd->clear_error();
> 
> So you must see the point. The NOT-KILLED-status caller's intention is to neither
> send any error back to the client neither affect the event instance with that error.
> Having 
>    thd->clear_error() 
> be called in front of
>    Query_log_event qinfo(... THD::NOT_KILLED);
> would be a better way of fixing this issue.
> 
> I suggest to analyze pieces of code where NOT-KILLED-status is not prepared with
> thd->clear_error() and consider to fixing that.
> 

No, this is not correct, NO_KILLED means only ignore the KILLED errors
(ER_QUERY_INTERRUPTED/ER_SERVER_SHUTDOWN), but other errors should not
be ignored. thd->clear_error() would clear all errors.

> 
> >        2) Add IF EXISTS to the DROP DATABASE d2 statement
> 
> This sounds fine because killing happens to be ineffective.
> 
> >        3) Temporarily disabled testing DROP FUNCTION/PROCEDURE IF EXISTS.
> 
> Please never be shy to say it ^ once again. Q: why? A: bug#43353.
> 

OK :)

> 
> cheers,
> 
> Andrei
> 
> >      @ mysql-test/t/rpl_killed_ddl.test
> >         DATABASE d2 might not exists, add IF EXITS to the DROP statement
> >
> >     M  mysql-test/r/rpl_killed_ddl.result
> >     M  mysql-test/t/rpl_killed_ddl.test
> >     M  sql/log_event.cc
> > === modified file 'mysql-test/r/rpl_killed_ddl.result'
> > --- a/mysql-test/r/rpl_killed_ddl.result	2009-03-27 05:19:50 +0000
> > +++ b/mysql-test/r/rpl_killed_ddl.result	2009-09-28 05:42:33 +0000
> > @@ -53,7 +53,7 @@ source include/diff_master_slave.inc;
> >  DROP DATABASE d1;
> >  source include/kill_query.inc;
> >  source include/diff_master_slave.inc;
> > -DROP DATABASE d2;
> > +DROP DATABASE IF EXISTS d2;
> >  source include/kill_query.inc;
> >  source include/diff_master_slave.inc;
> >  CREATE FUNCTION f2 () RETURNS INT DETERMINISTIC
> > @@ -63,10 +63,7 @@ source include/diff_master_slave.inc;
> >  ALTER FUNCTION f1 SQL SECURITY INVOKER;
> >  source include/kill_query.inc;
> >  source include/diff_master_slave.inc;
> > -DROP FUNCTION IF EXISTS f1;
> > -source include/kill_query.inc;
> > -source include/diff_master_slave.inc;
> > -DROP FUNCTION IF EXISTS f2;
> > +DROP FUNCTION f1;
> >  source include/kill_query.inc;
> >  source include/diff_master_slave.inc;
> >  CREATE PROCEDURE p2 (OUT rows INT)
> > @@ -79,10 +76,7 @@ source include/diff_master_slave.inc;
> >  ALTER PROCEDURE p1 SQL SECURITY INVOKER COMMENT 'return rows of table t1';
> >  source include/kill_query.inc;
> >  source include/diff_master_slave.inc;
> > -DROP PROCEDURE IF EXISTS p1;
> > -source include/kill_query.inc;
> > -source include/diff_master_slave.inc;
> > -DROP PROCEDURE IF EXISTS p2;
> > +DROP PROCEDURE p1;
> >  source include/kill_query.inc;
> >  source include/diff_master_slave.inc;
> >  CREATE TABLE t2 (b int);
> >
> > === modified file 'mysql-test/t/rpl_killed_ddl.test'
> > --- a/mysql-test/t/rpl_killed_ddl.test	2009-03-27 05:19:50 +0000
> > +++ b/mysql-test/t/rpl_killed_ddl.test	2009-09-28 05:42:33 +0000
> > @@ -123,7 +123,7 @@ source include/kill_query_and_diff_maste
> >  send DROP DATABASE d1;
> >  source include/kill_query_and_diff_master_slave.inc;
> >  
> > -send DROP DATABASE d2;
> > +send DROP DATABASE IF EXISTS d2;
> >  source include/kill_query_and_diff_master_slave.inc;
> >  
> >  ######## FUNCTION ########
> > @@ -139,13 +139,21 @@ source include/kill_query_and_diff_maste
> >  
> >  # function f1 probably does not exist because the ALTER query was
> >  # killed
> > -send DROP FUNCTION IF EXISTS f1;
> > +send DROP FUNCTION f1;
> >  source include/kill_query_and_diff_master_slave.inc;
> >  
> >  # function f2 probably does not exist because the CREATE query was
> >  # killed
> > -send DROP FUNCTION IF EXISTS f2;
> > -source include/kill_query_and_diff_master_slave.inc;
> > +#
> > +# Temporarily disabled. Because of BUG#43353, KILL the query may
> > +# result in function not found, and for 5.1, DROP statements will be
> > +# logged if the function is not found on master, so the following DROP
> > +# FUNCTION statement may be interrupted and not drop the function on
> > +# master, but still get logged and executed on slave and cause
> > +# inconsistence. Also disable the following DROP PROCEDURE IF EXITS
> > +# below.
> > +#send DROP FUNCTION IF EXISTS f2;
> > +#source include/kill_query_and_diff_master_slave.inc;
> >  
> >  ######## PROCEDURE ########
> >  
> > @@ -163,11 +171,12 @@ source include/kill_query_and_diff_maste
> >  send ALTER PROCEDURE p1 SQL SECURITY INVOKER COMMENT 'return rows of table
> t1';
> >  source include/kill_query_and_diff_master_slave.inc;
> >  
> > -send DROP PROCEDURE IF EXISTS p1;
> > +send DROP PROCEDURE p1;
> >  source include/kill_query_and_diff_master_slave.inc;
> >  
> > -send DROP PROCEDURE IF EXISTS p2;
> > -source include/kill_query_and_diff_master_slave.inc;
> > +# Temporarily disabled, see comment above for DROP FUNCTION IF EXISTS
> > +#send DROP PROCEDURE IF EXISTS p2;
> > +#source include/kill_query_and_diff_master_slave.inc;
> >  
> >  ######## TABLE ########
> >  
> >
> > === modified file 'sql/log_event.cc'
> > --- a/sql/log_event.cc	2009-08-12 03:54:05 +0000
> > +++ b/sql/log_event.cc	2009-09-28 05:42:33 +0000
> > @@ -1368,6 +1368,16 @@ Query_log_event::Query_log_event(THD* th
> >      ((thd_arg->system_thread & SYSTEM_THREAD_DELAYED_INSERT) ? 0 :
> >       thd->killed_errno());
> >    
> > +
> > +  /* thd_arg->main_da.sql_errno() might be ER_SERVER_SHUTDOWN or
> > +     ER_QUERY_INTERRUPTED, So here we need to make sure that
> > +     error_code is not set to these errors when specified NOT_KILLED
> > +     by the caller
> > +  */
> > +  if ((killed_status_arg == THD::NOT_KILLED) &&
> > +      (error_code == ER_SERVER_SHUTDOWN || error_code ==
> ER_QUERY_INTERRUPTED))
> > +    error_code= 0;
> > +
> >    time(&end_time);
> >    exec_time = (ulong) (end_time  - thd->start_time);
> >    catalog_len = (catalog) ? (uint32) strlen(catalog) : 0;
> >
> >
> > # Bazaar merge directive format 2 (Bazaar 0.90)
> > # revision_id: zhenxing.he@stripped
> > # target_branch: file:///media/sdb2/hezx/work/mysql/bzrwork/b45520\
> > #   /5.0-bugteam/
> > # testament_sha1: 11db89fad896dd445f04f7552fabac5c1a0d1286
> > # timestamp: 2009-09-28 13:43:18 +0800
> > # base_revision_id: joro@stripped
> > # 
> > # Begin bundle
> > IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWY6ixVwAA+9fgHAQWXf//3/v
> > //C////0YAlnfUdzAAyHdGN0wHzaj0V00A4SRTQmqab0mmU/U8mk9KHhoKPKDRoabUwgAHqaCVDS
> > T1T2TQpvSg00aAAANGgNAAAAOGmmCGQ00yMmEA00AYTRpkwAIGgkSECp+igeaKb1I9E9IHqGgBoN
> > ANPUaBoBtSITJHqPUMQ9E02oPUaNADQNDIADTQCSQCATEABTJp6J6minlG0nqGQ9TQHpMmj1LsYG
> > F8NOD301V2TTsDjDN9O0rN2P73qvrFo/NcU/ioeQkwcnNks5+wn354UlS5AYLrccSwMEUAFhLtpH
> > Dljscxy1zy0h2LcAvYBckX8nJVQ7AalkTbbSbDN3dojjGlpAz8IjDBizu9zmZtMyhCFIE3xbRk4n
> > WON3NETxK8+OD4OsTdNVeSCqcNz2YQUjxoLEb79PkG7WGq++78oNNHKVkqy7l9JVwZzOfSPfR1NN
> > VtRfZN9cF1MrVJfW6hVKORTAkVN9GKN+PqKk+9qWde1RMsmaWryLHh/TTDVVS5qVwz9I8tVt0ttt
> > TER0TUbGY18a1SZQalqq1j863SOYucrphUOYTvFMdXwyGLbT8ddxsy5yBCwchPwjZM97j2arfJIc
> > A9xtcknoX42M+K0B3zUQEjYg3DBzaP0xJ7H9umfXUclA0kOe5COGLHqg7zQGMJKqw8ZiANv+Fgjl
> > v6/FdK/TA+Jke+g67Oo/7VHAnf43LqgMUKhanepyPPejf5TPuF5FjNZ7YrDgad1qQzAzdzC5vl9u
> > si+TSkRKPQh46AxR48ecEozPzNpKlCIh0FRENZ/wrohDIUiYTtrLSBMedHnM5H8SgT0fvHqxkWs1
> > 0x5lNOINQJo6Z1I+d7pwqTvgxLKctJri9STrHOUQcaxvem6lo6375lUFBjPPNercTzMyZFx1+xmt
> > DUbR4v0VFZZfbQ2q2BROoJE9zLWiIigjaVG6VoyrLSs2GYi/VOoepFVVUhVmeNxtJmJ1Ed/YsVZw
> > EdliU3mWtZOLisQ2octIwyKbjU86d6wETVpIGHVmwcUkSRIY2z0DWkxVcPXrER/VjflDjZYxfF3c
> > 8JmJAwIDhDFcz5NyKHEuI7CZTUVkHHjaHEZ5Mv6lSyVZB9eBtJdMoERZpzEiUYiYpYNqE0bqEzkn
> > FECumy3pPaVCDhZ+uLQsepKrxRCL1zIjCiMCJMSym2l3L0BuTjI0l/j3c1hbkpjscTE2GBHAoh5X
> > /NxvlQlcai6NcTF7IcMY4Dj6rvM6uBU6fQ0JFkpKJmSN17N69BXkWmixjWuSzIEi00lVRbtKWmSE
> > W2OQmHToZEDItJkyhE4qupFhydCkvdb6IKl1cSqDwc5D75OCYqrScoytxAzUZ/OZu3AGtqXEu9bJ
> > WD2SIbxZmGFsTGFZvSFXlileJpYPB9DrLoTiOpF7KjGjdlKijGNj+aJYUjlE/MlIDbJcDvBlzBsO
> > FwyoXEfEGqoVPepNraJooH4Cza7mYv8g0HKgaVRZYG5NB90f0KJcyCcmzKi1GieCOYZhrUnrVINI
> > mSZAbA3hNJaGJ5MFQXBscLX9BVICAWJgRkg0IoBVek1sEWA1YY4KqdyVWAmaNaitp+gcj80sQyFx
> > BwUDNA4Kt4XVECzGXPoFa4Ldas8GNSYcgIcXQOQhc7Yh6wePUzA9MIiODCLCfkN42NXmHHU4Mgv3
> > xQiUXjqIJxUHr3t87nQpqkUxE0YbSimLiEQI12d8JWYYjmQzLr7yOVRlu4jGKy58Bu3HZZ1kwxfF
> > QM+KgK2LFVKLfc3f7gBtPNjAvOFigMfM0kCvkMsk3zTj8Chccynfacn2Z16nsIwIj/QK9AYEAlNz
> > Mi4cDkwwwM3BNqkqxisuMh1Ec5oiaiG8kI6WCIDLReXHy0POZER80axtpJQOYaQrEVCGcp8esTAc
> > e/68oRxfPaokmNhVQhT1UJr/BXXBw6uhUPWv2bzE7zw9Wwn/b3ZjVm8sO4jf51MdkVGtC8zyo4zR
> > EVRKXgezidjuzr6usqbg7lLBkk7lYR06Yc/bKSOE0E6QVhK4NIOHOCFBlxKxpOIFTJTT7st/xdHO
> > iNE6q6cxEDMphPlJR6FVIQpKkl1NO28VC0oLS2CvashKMEwsDfZadq1SosOA5XyEBHhJHV8RPHip
> > cmYS9OA6Sh2QciFUSSszs/K3WduthQ8+BmdHdwDOJedCo2HdUhaqJXynDFtKlYhRXWxaxr9d0tSM
> > UpgqC0+k8VnMFVTuQHcXp5YGiG0epCh6POqjOQuU/PrR/OiZBRPR20hwEPLJM7PKC3PEfCgjy7jt
> > vPJUNpVvu/AbkIb27Xi6+AjKwxGPWZjwNAiKIlviKia+utamgnCYlUdW00Z5DrSB4GSDTvGo3WlD
> > efZUvPAlgXtN9E3I8dCA1LHToW46pxBGXRg18/DAmpXiNF57LKwsVvAjuOEnEqXFTnMDDnP5b0A9
> > EWDBHteSm5SB6Hg4eJxMnOAIdO8tya60ufonZekXNNgNUgS7UY7zvj3SUnPYeVabV0E2BYI6I+Cq
> > JJBHAZhmQeV0KgIhIcKhZiGHnwQPgLamCtrxmUramM0ilUKGz2SoouCezFsQlMVvMxEXKJJY9HXX
> > Jh52Nxd8tWkjYqxjnbkeBv0nRg73EGO8aAmGA0ssh7EnXTeI9xWPPwyWDUqFhXCuWfRpksKtfTFU
> > lzi/Ym92MiC5jYSc4ZhuQ7MweHMhK80nQg8ajuFA9HFYymwY7PxZmyeRcGoKxGxy870TeXmSfvtN
> > gbGguPokxgrzcM4CSLU5+4IPxFDUPR9UBCJAlmzbnvHVoxKpQga3wLJUeytisXu8u6dpBR4CYQ90
> > d9sYoZbWJ6Q1VjsknTcAfHLkVQG6r9v2koiiK1SYcxNka608K15r2iMcz06CHYLTxGPWwIZYp11z
> > x5rUESxtfIY4rJeBedu0RHLzr7FkDyz3hkNE81d02Y38XCTjQbRGgmQS01My2s20ba3BQJKESKpm
> > m4lNOFu+0zGlDK5PPev5jup/4u5IpwoSEdRYq4A=
>