Re: bk commit into 5.0 tree (cmiller:1.2624) BUG#36570

[Davi Arnaut]

Hi Chad,

OK to push after the minor comments below are addressed.

Chad MILLER wrote:
> ChangeSet@stripped, 2008-05-15 19:13:24-04:00, cmiller@stripped +8 -0
>   Bug#36570: Parse error of CREATE PROCEDURE stmt with comments on \
>   	slave
>   
>   The stored-routine code took the contents of the (lowest) parser
>   and copied it directly to the binlog, which causes problems if there
>   is a special case of interpretation at the parser level -- which 
>   there is, in the "/*!VER */" comments.  The trailing "*/" caused
>   errors on the slave, naturally.
>   
>   Now, since by that point we have /properly/ created parse-tree (as 
>   the rest of the server should do!) for the stored-routine CREATE, we
>   can construct a perfect statement from that information, instead of
>   writing uncertain information from an unknown parser state.  
>   Fortunately, there's already a function nearby that does exactly 
>   that.
>   ---
>   Update for Bug#36570.  Qualify routine names with db name when
>   writing to the binlog ONLY if the source text is qualified.
> 
>   mysql-test/r/binlog_innodb.result@stripped, 2008-05-15 19:13:22-04:00,
> cmiller@stripped +1 -1
>     Offsets changed due to quoting.
>     ---
>     New offset to account for db-qualified names.
> 
>   mysql-test/r/ctype_cp932_binlog.result@stripped, 2008-05-15 19:13:22-04:00,
> cmiller@stripped +4 -4
>     Offsets changed due to quoting.
>     ---
>     Qualify routine names with DB.  Offsets change also.
> 
>   mysql-test/r/mysqlbinlog.result@stripped, 2008-05-15 19:13:22-04:00,
> cmiller@stripped +1 -1
>     Case changed in result due to interpretation of data instead of 
>     literal recitation.
>     ---
>     Qualify procedure name with db.
> 
>   mysql-test/r/rpl_sp.result@stripped, 2008-05-15 19:13:22-04:00,
> cmiller@stripped +67 -32
>     Offsets changed due to quoting.  Added tests.
>     ---
>     Qualify routine names with DB if qualified in query.  Offsets change also.
> 
>   mysql-test/t/rpl_sp.test@stripped, 2008-05-15 19:13:22-04:00,
> cmiller@stripped +44 -0
>     Add version-limiting quotes to exercise bug#36570.  Test that 
>     backtick-quoted identifiers and labels work also.
>     ---
>     Use different db to show qualification works.  Qualify routine names
>     with DB if qualified in query.
> 
>   sql/sp.cc@stripped, 2008-05-15 19:13:22-04:00, cmiller@stripped +39 -23
>     In create_string, we may not have a sp_name parameter yet, so
>     instead pass the char* and length of the only member we'd get out
>     of it.
>     
>     Having done that, we can use the same function to write the 
>     CREATE (FUNC|TRIG|PROC) statement to the binlog as we always used
>     to display the statement to the user.
>     ---
>     Make the db name part of the CREATE string if it is specified.
>     
>     Specify it in part of writing to the binlog when creating a new
>     routine.
> 
>   sql/sp_head.cc@stripped, 2008-05-15 19:13:22-04:00, cmiller@stripped +4 -0
>     Set the sp_head m_explicit_name member as the sp_name member is set.  
>     We can not peek at this later, as the sp_name is gone by then.
> 
>   sql/sp_head.h@stripped, 2008-05-15 19:13:22-04:00, cmiller@stripped +1 -0
>     Add a member to track whether the name is qualified with the 
>     database.
> 

[..]

> diff -Nrup a/mysql-test/t/rpl_sp.test b/mysql-test/t/rpl_sp.test
> --- a/mysql-test/t/rpl_sp.test	2008-01-31 06:17:25 -05:00
> +++ b/mysql-test/t/rpl_sp.test	2008-05-15 19:13:22 -04:00
> @@ -577,3 +577,47 @@ set global log_bin_trust_function_creato
>  drop database mysqltest;
>  drop database mysqltest2;
>  sync_slave_with_master;
> +
> +#
> +# Bug#36570: Parse error of CREATE PROCEDURE stmt with comments on slave
> +#
> +connection master;
> +use test;
> +delimiter |;
> +
> +/*!50001 create procedure `mysqltestbug36570_p1`() */
> +begin
> +	select 1;
> +end|
> +
> +use mysql|
> +create procedure test.` mysqltestbug36570_p2`(/*!50001 a int*/)`label`:
> +begin
> +	select a;
> +end|
> +
> +/*!50001 create function test.mysqltestbug36570_f1() */
> +	returns int
> +	/*!50001 deterministic */
> +begin
> +	return 3;
> +end|
> +use test|
>
> +delimiter ;|
> +
> +sync_slave_with_master;
> +connection slave;
> +--replace_column 5 t 6 t
> +show procedure status like '%mysqltestbug36570%';
> +show create procedure ` mysqltestbug36570_p2`;
> +call ` mysqltestbug36570_p2`(42);
> +
> +--replace_column 5 t 6 t
> +show function status like '%mysqltestbug36570%';
> +
> +connection master;

Please show the binlog events.

> +use test;
> +drop procedure mysqltestbug36570_p1;
> +drop procedure ` mysqltestbug36570_p2`;
> +drop function mysqltestbug36570_f1;
> diff -Nrup a/sql/sp.cc b/sql/sp.cc
> --- a/sql/sp.cc	2008-02-19 10:27:17 -05:00
> +++ b/sql/sp.cc	2008-05-15 19:13:22 -04:00
> @@ -25,7 +25,8 @@
>  static bool
>  create_string(THD *thd, String *buf,
>  	      int sp_type,
> -	      sp_name *name,
> +	      const char *db, ulong dblen,
> +	      const char *name, ulong namelen,
>  	      const char *params, ulong paramslen,
>  	      const char *returns, ulong returnslen,
>  	      const char *body, ulong bodylen,

OK.

> @@ -426,12 +427,13 @@ db_load_routine(THD *thd, int type, sp_n
>     */
>  
>    if (!create_string(thd, &defstr,
> -		     type,
> -		     name,
> -		     params, strlen(params),
> -		     returns, strlen(returns),
> -		     body, strlen(body),
> -		     &chistics, &definer_user_name, &definer_host_name))
> +                     type,
> +                     NULL, 0,
> +                     name->m_name.str, name->m_name.length,
> +                     params, strlen(params),
> +                     returns, strlen(returns),
> +                     body, strlen(body),
> +                     &chistics, &definer_user_name,
> &definer_host_name))
>    {
>      ret= SP_INTERNAL_ERROR;
>      goto end;
> @@ -518,6 +520,7 @@ db_create_routine(THD *thd, int type, sp
>    DBUG_ENTER("db_create_routine");
>    DBUG_PRINT("enter", ("type: %d name: %.*s",type,sp->m_name.length,
>                         sp->m_name.str));
> +  String retstr(64);
>  
>    if (!(table= open_proc_table_for_update(thd)))
>      ret= SP_OPEN_TABLE_FAILED;
> @@ -568,7 +571,6 @@ db_create_routine(THD *thd, int type, sp
>        store(sp->m_params.str, sp->m_params.length, system_charset_info);
>      if (sp->m_type == TYPE_ENUM_FUNCTION)
>      {
> -      String retstr(64);
>        sp_returns_type(thd, retstr, sp);
>        table->field[MYSQL_PROC_FIELD_RETURNS]->
>  	store(retstr.ptr(), retstr.length(), system_charset_info);
> @@ -625,13 +627,21 @@ db_create_routine(THD *thd, int type, sp
>  
>        String log_query;
>        log_query.set_charset(system_charset_info);
> -      log_query.append(STRING_WITH_LEN("CREATE "));
> -      append_definer(thd, &log_query, &thd->lex->definer->user,
> -                     &thd->lex->definer->host);
> -      log_query.append(thd->lex->stmt_definition_begin,
> -                       (char *)sp->m_body_begin -
> -                       thd->lex->stmt_definition_begin +
> -                       sp->m_body.length);
> +
> +      if (!create_string(thd, &log_query,
> +                         sp->m_type,
> +                         (sp->m_explicit_name ? sp->m_db.str : NULL), 
> +                         (sp->m_explicit_name ? sp->m_db.length : 0), 
> +                         sp->m_name.str, sp->m_name.length,
> +                         sp->m_params.str, sp->m_params.length,
> +                         retstr.c_ptr(), retstr.length(),
> +                         sp->m_body.str, sp->m_body.length,
> +                         sp->m_chistics,
> &(thd->lex->definer->user),
> +                         &(thd->lex->definer->host)))
> +      {
> +        ret= SP_INTERNAL_ERROR;
> +        goto done;
> +      }

OK.

>        /* Such a statement can always go directly to binlog, no trans cache */
>        Query_log_event qinfo(thd, log_query.c_ptr(), log_query.length(), 0,
> @@ -1797,17 +1807,18 @@ sp_cache_routines_and_add_tables_for_tri
>   */
>  static bool
>  create_string(THD *thd, String *buf,
> -	      int type,
> -	      sp_name *name,
> -	      const char *params, ulong paramslen,
> -	      const char *returns, ulong returnslen,
> -	      const char *body, ulong bodylen,
> -	      st_sp_chistics *chistics,
> +              int type,
> +              const char *db, ulong dblen,
> +              const char *name, ulong namelen,
> +              const char *params, ulong paramslen,
> +              const char *returns, ulong returnslen,
> +              const char *body, ulong bodylen,
> +              st_sp_chistics *chistics,
>                const LEX_STRING *definer_user,
>                const LEX_STRING *definer_host)

Good.

>  {
>    /* Make some room to begin with */
> -  if (buf->alloc(100 + name->m_qname.length + paramslen + returnslen + bodylen
> +
> +  if (buf->alloc(100 + dblen + 1 + namelen + paramslen + returnslen + bodylen +
>  		 chistics->comment.length + 10 /* length of " DEFINER= "*/ +
>                   USER_HOST_BUFF_SIZE))
>      return FALSE;
> @@ -1818,7 +1829,12 @@ create_string(THD *thd, String *buf,
>      buf->append(STRING_WITH_LEN("FUNCTION "));
>    else
>      buf->append(STRING_WITH_LEN("PROCEDURE "));
> -  append_identifier(thd, buf, name->m_name.str, name->m_name.length);
> +  if (dblen > 0)

As dblen is unsigned, if (dblen)

> +  {
> +    append_identifier(thd, buf, db, dblen);
> +    buf->append('.');
> +  }
> +  append_identifier(thd, buf, name, namelen);
>    buf->append('(');
>    buf->append(params, paramslen);
>    buf->append(')');
> diff -Nrup a/sql/sp_head.cc b/sql/sp_head.cc
> --- a/sql/sp_head.cc	2008-01-23 16:04:18 -05:00
> +++ b/sql/sp_head.cc	2008-05-15 19:13:22 -04:00
> @@ -522,6 +522,8 @@ sp_head::init(LEX *lex)
>    m_qname.str= NULL;
>    m_qname.length= 0;
>  
> +  m_explicit_name= false;

s/false/FALSE/

> +
>    m_db.str= NULL;
>    m_db.length= 0;
>  
> @@ -563,6 +565,8 @@ sp_head::init_sp_name(THD *thd, sp_name 
>    m_name.length= spname->m_name.length;
>    m_name.str= strmake_root(thd->mem_root, spname->m_name.str,
>                             spname->m_name.length);
> +
> +  m_explicit_name= spname->m_explicit_name;
>  
>    if (spname->m_qname.length == 0)
>      spname->init_qname(thd);
> diff -Nrup a/sql/sp_head.h b/sql/sp_head.h
> --- a/sql/sp_head.h	2008-01-23 15:26:38 -05:00
> +++ b/sql/sp_head.h	2008-05-15 19:13:22 -04:00
> @@ -121,6 +121,7 @@ public:
>    st_sp_chistics *m_chistics;
>    ulong m_sql_mode;		// For SHOW CREATE and execution
>    LEX_STRING m_qname;		// db.name
> +  bool m_explicit_name;                /**< Prepend the db name? */
>    /**
>      Key representing routine in the set of stored routines used by statement.
>      [routine_type]db.name\0
> 

OK.

Regards,

-- 
Davi Arnaut, Software Engineer
MySQL Server Runtime Team
Database Group, Sun Microsystems