List:Commits« Previous MessageNext Message »
From:He Zhenxing Date:July 23 2010 6:43am
Subject:Re: bzr commit into mysql-trunk-bugfixing branch (luis.soares:3133)
Bug#55387
View as plain text  
Hi Luis,

Thank you for fixing the problem, patch approved!

Luis Soares wrote:
> #At file:///home/lsoares/Workspace/bzr/work/bugfixing/55387/mysql-trunk-bugfixing/
> based on revid:jonathan.perkin@stripped
> 
>  3133 Luis Soares	2010-07-22
>       BUG#55387: binlog.binlog_tmp_table crashes the server
>                  sporadically
>       
>       There are two problems:
>       
>       1. When closing temporary tables, during the THD clean up - and
>          after the session connection was already closed, there is a
>          chance we can push an error into the THD diagnostics area, if
>          the writing of the implicit DROP event to the binary log fails
>          for some reason. As a consequence an assertion can be
>          triggered, because at that point the diagnostics area is
>          already set.
>       
>       2. Using push_warning with MYSQL_ERROR::WARN_LEVEL_ERROR is a 
>          bug.
>       
>       Given that close_temporary_tables is mostly called from
>       THD::cleanup - ie, with the session already closed, we fix
>       problem #1 by allowing the diagnostics area to be
>       overwritten. There is one other place in the code that calls
>       close_temporary_tables - while applying Start_log_event_v3. To
>       cover that case, we make close_temporary_tables to return the
>       error, thus, propagating upwards in the stack.
>       
>       To fix problem #2, we replace push_warning with sql_print_error.
>      @ sql/log_event.cc
>         Added handling of error returned by close_temporary_tables to
>         Start_log_event_v3::do_apply_event.
>      @ sql/sql_base.cc
>         Three changes to close_temporary_tables:
>         1. it returns a boolean now (instead of void)
>         2. it uses sql_print_error instead of push_warning when writing to
>            binary log fails
>         3. we set can_overwrite_status before writing to the binary log,
>            thence not risking triggering an assertion by any other push
>            into diagnostics area happening inside mysql_bin_log.write.
>      @ sql/sql_base.h
>         Changed the interface of close_temporary_tables so that it returns
>         bool instead of void.
> 
>     modified:
>       sql/log_event.cc
>       sql/sql_base.cc
>       sql/sql_base.h
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc	2010-07-15 13:47:50 +0000
> +++ b/sql/log_event.cc	2010-07-22 15:56:50 +0000
> @@ -3719,6 +3719,7 @@ bool Start_log_event_v3::write(IO_CACHE*
>  int Start_log_event_v3::do_apply_event(Relay_log_info const *rli)
>  {
>    DBUG_ENTER("Start_log_event_v3::do_apply_event");
> +  int error= 0;
>    switch (binlog_version)
>    {
>    case 3:
> @@ -3731,7 +3732,7 @@ int Start_log_event_v3::do_apply_event(R
>      */
>      if (created)
>      {
> -      close_temporary_tables(thd);
> +      error= close_temporary_tables(thd);
>        cleanup_load_tmpdir();
>      }
>      else
> @@ -3759,7 +3760,7 @@ int Start_log_event_v3::do_apply_event(R
>          Can distinguish, based on the value of 'created': this event was
>          generated at master startup.
>        */
> -      close_temporary_tables(thd);
> +      error= close_temporary_tables(thd);
>      }
>      /*
>        Otherwise, can't distinguish a Start_log_event generated at
> @@ -3771,7 +3772,7 @@ int Start_log_event_v3::do_apply_event(R
>      /* this case is impossible */
>      DBUG_RETURN(1);
>    }
> -  DBUG_RETURN(0);
> +  DBUG_RETURN(error);
>  }
>  #endif /* defined(HAVE_REPLICATION) && !defined(MYSQL_CLIENT) */
>  
> 
> === modified file 'sql/sql_base.cc'
> --- a/sql/sql_base.cc	2010-07-15 13:47:50 +0000
> +++ b/sql/sql_base.cc	2010-07-22 15:56:50 +0000
> @@ -1634,7 +1634,7 @@ static inline uint  tmpkeyval(THD *thd, 
>    creates one DROP TEMPORARY TABLE binlog event for each pseudo-thread 
>  */
>  
> -void close_temporary_tables(THD *thd)
> +bool close_temporary_tables(THD *thd)
>  {
>    DBUG_ENTER("close_temporary_tables");
>    TABLE *table;
> @@ -1642,9 +1642,10 @@ void close_temporary_tables(THD *thd)
>    TABLE *prev_table;
>    /* Assume thd->variables.option_bits has OPTION_QUOTE_SHOW_CREATE */
>    bool was_quote_show= TRUE;
> +  bool error= 0;
>  
>    if (!thd->temporary_tables)
> -    DBUG_VOID_RETURN;
> +    DBUG_RETURN(FALSE);
>  
>    if (!mysql_bin_log.is_open())
>    {
> @@ -1655,7 +1656,7 @@ void close_temporary_tables(THD *thd)
>        close_temporary(table, 1, 1);
>      }
>      thd->temporary_tables= 0;
> -    DBUG_VOID_RETURN;
> +    DBUG_RETURN(FALSE);
>    }
>  
>    /* Better add "if exists", in case a RESET MASTER has been done */
> @@ -1754,11 +1755,27 @@ void close_temporary_tables(THD *thd)
>        qinfo.db= db.ptr();
>        qinfo.db_len= db.length();
>        thd->variables.character_set_client= cs_save;
> -      if (mysql_bin_log.write(&qinfo))
> +
> +      thd->stmt_da->can_overwrite_status= TRUE;
> +      if ((error= (mysql_bin_log.write(&qinfo) || error)))
>        {
> -        push_warning(thd, MYSQL_ERROR::WARN_LEVEL_ERROR, MYF(0),
> -                     "Failed to write the DROP statement for temporary tables to
> binary log");
> +        /*
> +          If we're here following THD::cleanup, thence the connection
> +          has been closed already. So lets print a message to the
> +          error log instead of pushing yet another error into the
> +          stmt_da.
> +
> +          Also, we keep the error flag so that we propagate the error
> +          up in the stack. This way, if we're the SQL thread we notice
> +          that close_temporary_tables failed. (Actually, the SQL
> +          thread only calls close_temporary_tables while applying old
> +          Start_log_event_v3 events.)
> +        */
> +        sql_print_error("Failed to write the DROP statement for "
> +                        "temporary tables to binary log");
>        }
> +      thd->stmt_da->can_overwrite_status= FALSE;
> +
>        thd->variables.pseudo_thread_id= save_pseudo_thread_id;
>        thd->thread_specific_used= save_thread_specific_used;
>      }
> @@ -1771,7 +1788,8 @@ void close_temporary_tables(THD *thd)
>    if (!was_quote_show)
>      thd->variables.option_bits&= ~OPTION_QUOTE_SHOW_CREATE; /* restore option
> */
>    thd->temporary_tables=0;
> -  DBUG_VOID_RETURN;
> +
> +  DBUG_RETURN(error);
>  }
>  
>  /*
> 
> === modified file 'sql/sql_base.h'
> --- a/sql/sql_base.h	2010-06-11 15:28:18 +0000
> +++ b/sql/sql_base.h	2010-07-22 15:56:50 +0000
> @@ -222,7 +222,7 @@ int decide_logging_format(THD *thd, TABL
>  void free_io_cache(TABLE *entry);
>  void intern_close_table(TABLE *entry);
>  bool close_thread_table(THD *thd, TABLE **table_ptr);
> -void close_temporary_tables(THD *thd);
> +bool close_temporary_tables(THD *thd);
>  TABLE_LIST *unique_table(THD *thd, TABLE_LIST *table, TABLE_LIST *table_list,
>                           bool check_alias);
>  int drop_temporary_table(THD *thd, TABLE_LIST *table_list);
> 
> text/bzr-bundle 类型 附件
> (bzr/luis.soares@stripped)
> # Bazaar merge directive format 2 (Bazaar 0.90)
> # revision_id: luis.soares@stripped
> # target_branch: file:///home/lsoares/Workspace/bzr/work/bugfixing\
> #   /55387/mysql-trunk-bugfixing/
> # testament_sha1: 8b15a83382ff7cd6a410d78c4c936a9568c92589
> # timestamp: 2010-07-22 16:56:54 +0100
> # source_branch: /home/lsoares/Workspace/bzr/clones/mysql-trunk-\
> #   bugfixing
> # base_revision_id: jonathan.perkin@stripped\
> #   gqyrtmkgwrjfgsmc
> # 
> # Begin bundle
> IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWevzfBIABGZ/gFWQBABZ9///
> f+f/oL////RgCofXvd7t3nvn275kO1rmeddu7a7Zbdd6PPedq220dXbwkkJGCmTYjRkNT0qf6poy
> nqaflTAjGo/UJtNBqZogyQEnhNTyTKTZQ0ep6m0QeoGmQAAAGjQRTBMUxNDQNAGgDQAAAAAAAkRB
> E1NPUxD1NGpojano0TTINBoAADQGgiiQp5MiaT0TGpkaNNNNMmmTJkDQaAABoJIppkaJpiBop4CR
> pqbRGg0A0DIAB6RjOthBdpq03OO9tcG/TRGmblYTztxf319g/C8I3yS7r33cc/ax5foXRfWmyl/Z
> 8PDNkqTa2pjtiz2O6tv6xwkzfNw8mckDQcWas3W+dvRWzUytlCLdOVJGfhXkhGTJCKUAMmZMwW5j
> To78nDzncV3LnWlMzAMzMLdjPokQr+0ZdHXHTN6nB8HDo4YWpVY0HZkyVdXdxW9Sl2q2y1EBiOy4
> MDK9yAIxVAKAKnqkaJQQl5fSkqiQBIDE5RKQsg/bR1bTVdOxqKqCL8KDlGFveaC6cRojZngxPO51
> dH/Rn8OCmQgBOKn1564CzsrBTEl2JyTPJyMEF/O5V+kK5MzBk4XjsTOd68xsdYm+F08rePK+y2Wn
> FWGWriagaAWWVRffSbIkahQlZObJ4WtKChRTB9ObPddZWaBcKwljwBKLsFWQMqRmXAhzJUQqNo4I
> hoGBnyXgiurtuzKb/dneeJYbU+luTclD+XIXnynGFBraB/ppGInp5r0Wu6LIS8S92JN5PRMUxT2Y
> ObCEjclGtpVctQti4AFqyEp0bz3zs26wRfrOwzYMYlpn1Hm+HSoq6cEZbRuCK36C6vZEsKQwEXss
> Ka8pLapWcuxndOzStaJlrHdgSIQnWBieMZRMzXCh1hoBS2Vo6BGbNKttIKNMVy/ShrIsXKDd67yy
> DBk5QEXmZ9XIF625iQLggPs5FA37cBXpxzCiFVIufui4Kkn7DJ22Dny6CdOM816K4c+XCOwYrEPk
> QMwFVcror6hatFM8FJ70bL02rxmbdSK/GsdMT3VUb95JYpAxlUI0eOCRZtYbE8JaGWJORcOaXHwI
> bIC4ETjM4cE6QLOc5rodEt8qyxukC94WNRs5wFdQIk9AjBpKzCfAoUIG9oLhWiziMm5WDC4EaStZ
> /pc1mNloosIwND53GsuHC8zO0Z8dtZG0d4nI2zCELS4oBvcuxC03SsjRa1FKqEOZEi4veowKuCg6
> 50qgRN1AREnBytPMBiVtJamDsVU5RvgQ3W2Gene2wbNcraXqTKXItICKaS2PO6R4h8aXYJdFlhSX
> 5y4qOm+1bBbeXbljHELcbmpTEeRXUmag0HBibm1UvEZ1KRqpjGTb1vOMmZMIxEm8p8xhGlMJBov3
> g9jqirljoM3VtgYmcq3JRY221NhTTTh0GWsvGWbAxotYxSKRswsNksdZjVlQaPeay0YRaPnNp5jN
> ZlEPn+k1b8nHP0XrBrgiMlz4PKFDtQJCLyiqiXAYOsQ04scbb8fBDMnYBq6VQMfvksTDbFsE4bbw
> XScGMSukJkjsX38tvHNlVCYZmhtCp3aLiYw7l9pPv+DwBxylW2pGfwFh4E/6WNGM8yDwMExUFoV6
> SJMtvUz3MXNnwHHYkDuZ2ev0omEIxf3WHs9oer4wXIX0eoDYduplCSWGt7nTUNb02gyDbU1vJohW
> REx4GoDkOA46ziOY5CSTkOUvPKWHs6squp541KHO5FhhqwOW9zRFxfeHxZL5J24tfZAcFnBR5w96
> hPKkMVETwyc/RKZUNoHpKm9wC8WmLDWEqx3mWwXM7ml5FMoQgpYHMT6l0XkI17CNdauH9FxpEXjq
> VKMoNGm/emJe4vMVb1P6GsaJA8zhyQJw9gjaeXtJbDoaC1dGX7jc7FqA2sXy362YY4nWkfoI7iiB
> NZstxkXMbktM5nkAxoFJaRYGU5d3FRLkY+OzUpm9RPVq1+DvXgsnwg7mcSOstkceSK0LJdFiUbzk
> L2LTLrDVmNYP5QzW91dcpBVkb9jsyH0FeVDw3+6SzBqlibuCTkpDziO/Fk2V7r2jwzNYsVnzTPeh
> yZDLXWS2zOvLn+8z3GI6VSvCVtys7KLEKCvBaA6lAOamCINtyJNDLCdEojwJFlPPsosyk1+oN0ce
> WGj2+KKG1JOOLR0t2ye1pbcxNNJowfLNk8efoigaRaNS752M9Lh+k8UxJ6y2C1huzanXBxMCCR2b
> xGhEhu9yMQtuQM4sEpVnal/z6uguwdkMDUpFsOc3dQ4iuqEZqFiIBNv8eYLU9PXeurmTLUjg+peV
> dqlbGW8CjfP5Fl3hthRpEHa1DQSbzmRlSrGRZmKg03iYdl7DeWuftxBwBjliikKNnsiO6iVRDElu
> v+Pooj1YRF6DjcuZVJfUAMshjQtvkDq0H4aaA1Fy7trIL3wi3e1gWHeXIW8B4cRcCNR7wKA4eJnd
> zythFtQ1zBBHmmuYvO5MyYunCjyccbLmGZhR3sFrpG4U+dKcWCYPP8u1TWsLeR56lesl3K7Jlit+
> F6gyrXIWeztNoMHFDDM3QMl6CQtUF7YpL0GU7PMc2DBMgg6nQXUoDGalwqyq507KhiNFdR7vAvg9
> QoAKCMKBxes69D5CYGWjCG6d3OtCptGQFOxVp9CiinCgsW2lZVJrUcsJAuxC4ODOs2NneuvujEwW
> kSziqaVVQRnFo07rURWVCbKK6gJVyqqqCdOUrFEkUNa13stVi694NWLIVkUqldsBpiYNNisQnyZn
> kYN7N9FwwqX0p+cYJFq4tnS3ID5jzHixClrJVIC6NJZhPG6HNFbspob7AXQMQp1OGYYFqCJV4BzW
> 8Pf1uZjF7u1eWispXFcGr1oGnSdXTAUBo8bXvNl1ljS2gtNoMVGvI5JDDckbOByuo/7GKZLWuPsX
> BbZbaNkEDGJppLgxLnpQ9jHFGFxsDCNEEFDAmDRpcKTVuH1XhT40KCtWtRsGh3WS1u1b6PZEMQha
> w8TfbRO+UrGCAdvMZHYlT5q84PO9/G8DKEoYqzKR1UoNIpwRyTTFIzJLNska4sEhDogF+hekROmv
> 8bLDOKpNci/Bda2qFsH7aRHH5153uiI9nKt8qkqsNzFC/reiMRm5SQ9q5goL6T5cheUSei1aYZS2
> oGSYxJMRRijecykXWvKKFxizYm8l6Ewy1Mu1ITy3uCtvaWArJFGJ5Swdx5D0jxCqTRMb1jgZQo0E
> yr2UHUhbIWKM2MagUKB97oizRiYfOqhqWGG9Vbt/7G9e7UkYlZQJUT0VtB8+NHjY5YdInHiVanK8
> 4mDGTXTobiobyrVcL2o4SEdJsJeVyzvmNljSTLLkO5UI+BQs0tOkY9DFHNEHLWJcAwXStchp1HaU
> KrG8CWg5Hoh/xdyRThQkOvzfBIA=
> 
> 


Thread
bzr commit into mysql-trunk-bugfixing branch (luis.soares:3133)Bug#55387Luis Soares22 Jul
  • Re: bzr commit into mysql-trunk-bugfixing branch (luis.soares:3133)Bug#55387He Zhenxing23 Jul