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

[Andrei Elkin]

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.


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

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.


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


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=