List:Commits« Previous MessageNext Message »
From:Luís Soares Date:July 26 2009 11:30pm
Subject:Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3028)
Bug#41166
View as plain text  
Hi Alfranio,

  Nice work. Check comments below.

STATUS
------

  Approved.

REQUIRED CHANGES
----------------

  n/a

REQUESTS
--------

  n/a

SUGGESTIONS
-----------

  S1. Why not make the check, in Item_func_sp::execute_impl,
      according to the condition itself? I think it would be more
      readable:

       if ( !(m_sp->m_chistics->detistic || 
              access == SP_READS_SQL_DATA ||
              access == SP_NO_SQL) && 
            !trust_function_creators && ...

  S2. Is there a test for checking all these? If not, I would
      suggest to extend suites/rpl/t/rpl_sf.test to include the missing
      ones.

DETAILS
-------
  n/a

On Thu, 2009-07-16 at 21:38 +0000, Alfranio Correia wrote:
> #At
> file:///home/acorreia/workspace.sun/repository.mysql/bzrwork/bug-41166/mysql-5.1-bugteam/
> based on revid:ramil@stripped
> 
>  3028 Alfranio Correia	2009-07-16
>       BUG#41166 stored function requires "deterministic" if binlog_format is
> "statement"
>       
>       If the log_bin_trust_function_creators option is not defined, creating a
> stored
>       function requires either one of the modifiers DETERMINISTIC, NO SQL, or READS
>       SQL DATA. Executing a stored function should also follows the same rules if in
>       STATEMENT mode. However, this was not happening and a wrong error was being
>       printed out: ER_BINLOG_ROW_RBR_TO_SBR.
>       
>       The patch makes the creation and execution compatible and prints out the
> correct
>       error ER_BINLOG_UNSAFE_ROUTINE when a stored function without one of the
> modifiers
>       above is executed in STATEMENT mode.
> 
>     modified:
>       mysql-test/suite/rpl/r/rpl_sf.result
>       mysql-test/suite/rpl/t/rpl_sf.test
>       sql/item_func.cc
>       sql/sp.cc
>       sql/sp.h
> === modified file 'mysql-test/suite/rpl/r/rpl_sf.result'
> --- a/mysql-test/suite/rpl/r/rpl_sf.result	2007-06-27 12:28:02 +0000
> +++ b/mysql-test/suite/rpl/r/rpl_sf.result	2009-07-16 21:38:33 +0000
> @@ -19,5 +19,5 @@ fn16456()
>  timestamp
>  set binlog_format=STATEMENT;
>  select fn16456();
> -ERROR HY000: Slave running with --log-slave-updates must use row-based binary
> logging to be able to replicate row-based binary log events
> +ERROR HY000: This function has none of DETERMINISTIC, NO SQL, or READS SQL DATA in
> its declaration and binary logging is enabled (you *might* want to use the less safe
> log_bin_trust_function_creators variable)
>  drop function fn16456;
> 
> === modified file 'mysql-test/suite/rpl/t/rpl_sf.test'
> --- a/mysql-test/suite/rpl/t/rpl_sf.test	2007-06-27 12:28:02 +0000
> +++ b/mysql-test/suite/rpl/t/rpl_sf.test	2009-07-16 21:38:33 +0000
> @@ -55,7 +55,7 @@ select fn16456();
>  
>  set binlog_format=STATEMENT;
>  
> ---error ER_BINLOG_ROW_RBR_TO_SBR
> +--error ER_BINLOG_UNSAFE_ROUTINE
>  select fn16456();
>  
> 
> 
> === modified file 'sql/item_func.cc'
> --- a/sql/item_func.cc	2009-07-03 10:36:04 +0000
> +++ b/sql/item_func.cc	2009-07-16 21:38:33 +0000
> @@ -5990,6 +5990,9 @@ Item_func_sp::execute_impl(THD *thd)
>  #ifndef NO_EMBEDDED_ACCESS_CHECKS
>    Security_context *save_security_ctx= thd->security_ctx;
>  #endif
> +  enum enum_sp_data_access access=
> +    (m_sp->m_chistics->daccess == SP_DEFAULT_ACCESS) ?
> +     SP_DEFAULT_ACCESS_MAPPING : m_sp->m_chistics->daccess;
>  
>    DBUG_ENTER("Item_func_sp::execute_impl");
>  
> @@ -6007,11 +6010,13 @@ Item_func_sp::execute_impl(THD *thd)
>      Throw an error if a non-deterministic function is called while
>      statement-based replication (SBR) is active.
>    */
> +
>    if (!m_sp->m_chistics->detistic && !trust_function_creators
> &&
> +      (access == SP_CONTAINS_SQL || access == SP_MODIFIES_SQL_DATA) &&
>        (mysql_bin_log.is_open() &&
>         thd->variables.binlog_format == BINLOG_FORMAT_STMT))
>    {
> -    my_error(ER_BINLOG_ROW_RBR_TO_SBR, MYF(0));
> +    my_error(ER_BINLOG_UNSAFE_ROUTINE, MYF(0));
>      goto error;
>    }
>  
> 
> === modified file 'sql/sp.cc'
> --- a/sql/sp.cc	2009-05-30 13:32:28 +0000
> +++ b/sql/sp.cc	2009-07-16 21:38:33 +0000
> @@ -70,9 +70,6 @@ enum
>    MYSQL_PROC_FIELD_COUNT
>  };
>  
> -/* Tells what SP_DEFAULT_ACCESS should be mapped to */
> -#define SP_DEFAULT_ACCESS_MAPPING SP_CONTAINS_SQL
> -
>  /*************************************************************************/
>  
>  /**
> 
> === modified file 'sql/sp.h'
> --- a/sql/sp.h	2009-05-29 13:37:54 +0000
> +++ b/sql/sp.h	2009-07-16 21:38:33 +0000
> @@ -17,6 +17,9 @@
>  #ifndef _SP_H_
>  #define _SP_H_
>  
> +/* Tells what SP_DEFAULT_ACCESS should be mapped to */
> +#define SP_DEFAULT_ACCESS_MAPPING SP_CONTAINS_SQL
> +
>  // Return codes from sp_create_*, sp_drop_*, and sp_show_*:
>  #define SP_OK                 0
>  #define SP_KEY_NOT_FOUND     -1
> 
-- 
Luís

Thread
bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3028)Bug#41166Alfranio Correia16 Jul
  • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3028)Bug#41166Luís Soares27 Jul
  • Re: bzr commit into mysql-5.1-bugteam branch (alfranio.correia:3028)Bug#41166He Zhenxing27 Jul