Re: bk commit into 5.0 tree (davi:1.2524) BUG#28318

[Marc Alff]

Hi Davi

Ok to push,
please see below for minor changes

Thanks,
Marc


Davi Arnaut wrote:
> Below is the list of changes that have just been committed into a local
> 5.0 repository of davi. When davi does a push these changes will
> be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
>
> ChangeSet@stripped, 2007-09-13 13:13:14-03:00, davi@stripped +8 -0
>   Bug#28318 CREATE FUNCTION (UDF) requires a schema
>   Bug#29816 Syntactically wrong query fails with misleading error message
>   
>   The core problem is that an SQL-invoked function name can be a <schema
>   qualified routine name> that contains no <schema name>, but the mysql
>   parser insists that all stored procedures (function, procedures and
>   triggers) must have a <schema name>, which is not true for functions.
>   This problem is especially visible when trying to create a function
>   or when a query contains a syntax error after a function call (in the
>   same query), both will fail with a "No database selected" message if
>   the session is not attached to a particular schema, but the first
>   one should succeed and the second fail with a "syntax error" message.
>   
>   Part of the fix is to revamp the sp name handling so that a schema
>   name may be omitted for functions -- this means that the internal
>   function name representation may not have a dot, which represents
>   that the function doesn't have a schema name. The other part is
>   to place schema checks after the type (function, trigger or procedure)
>   of the routine is known.
>
>   mysql-test/r/sp-error.result@stripped, 2007-09-13 13:13:10-03:00, davi@stripped
> +13 -0
>     Add test case result for Bug#29816
>
>   mysql-test/r/udf.result@stripped, 2007-09-13 13:13:10-03:00, davi@stripped +7 -0
>     Add test case result for Bug#28318
>
>   mysql-test/t/sp-error.test@stripped, 2007-09-13 13:13:10-03:00, davi@stripped +20
> -0
>     Add test case for Bug#29816
>
>   mysql-test/t/udf.test@stripped, 2007-09-13 13:13:10-03:00, davi@stripped +14 -0
>     Add test case for Bug#28318
>
>   sql/sp.cc@stripped, 2007-09-13 13:13:11-03:00, davi@stripped +3 -10
>     Copy the (last) nul byte of the stored routine key and move name parsing
>     code to the sp_name class constructor.
>
>   sql/sp_head.cc@stripped, 2007-09-13 13:13:11-03:00, davi@stripped +28 -3
>     Revamp routine name parsing for when no schema is specified and
>     omit dot from the qualified name if the routine is not associated
>     with a scheme name.
>
>   sql/sp_head.h@stripped, 2007-09-13 13:13:11-03:00, davi@stripped +1 -10
>     Name parsing got bigger, uninline by moving to a single unit -- the sp_head.cc
>     file.
>
>   sql/sql_yacc.yy@stripped, 2007-09-13 13:13:11-03:00, davi@stripped +26 -5
>     Only copy the schema name if one is actually set and check for schema
>     name presence only where it's necessary.
>
> diff -Nrup a/mysql-test/r/sp-error.result b/mysql-test/r/sp-error.result
> --- a/mysql-test/r/sp-error.result	2007-06-22 06:55:46 -03:00
> +++ b/mysql-test/r/sp-error.result	2007-09-13 13:13:10 -03:00
> @@ -1452,3 +1452,16 @@ end
>  until true end repeat retry;
>  end//
>  ERROR 42000: LEAVE with no matching label: retry
> +DROP DATABASE IF EXISTS mysqltest;
> +CREATE DATABASE mysqltest;
> +USE mysqltest;
> +DROP DATABASE mysqltest;
> +SELECT inexistent(), 1 + ,;
> +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds
> to your MySQL server version for the right syntax to use near '' at line 1
> +SELECT inexistent();
> +ERROR 42000: FUNCTION inexistent does not exist
> +SELECT .inexistent();
> +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds
> to your MySQL server version for the right syntax to use near '()' at line 1
> +SELECT ..inexistent();
> +ERROR 42000: You have an error in your SQL syntax; check the manual that corresponds
> to your MySQL server version for the right syntax to use near '.inexistent()' at line 1
> +USE test;
> diff -Nrup a/mysql-test/r/udf.result b/mysql-test/r/udf.result
> --- a/mysql-test/r/udf.result	2007-06-18 18:55:07 -03:00
> +++ b/mysql-test/r/udf.result	2007-09-13 13:13:10 -03:00
> @@ -296,4 +296,11 @@ Qcache_queries_in_cache	0
>  drop table t1;
>  drop function metaphon;
>  set GLOBAL query_cache_size=default;
> +DROP DATABASE IF EXISTS mysqltest;
> +CREATE DATABASE mysqltest;
> +USE mysqltest;
> +DROP DATABASE mysqltest;
> +CREATE FUNCTION metaphon RETURNS STRING SONAME "UDF_EXAMPLE_LIB";
> +DROP FUNCTION metaphon;
> +USE test;
>  End of 5.0 tests.
> diff -Nrup a/mysql-test/t/sp-error.test b/mysql-test/t/sp-error.test
> --- a/mysql-test/t/sp-error.test	2007-06-22 06:55:46 -03:00
> +++ b/mysql-test/t/sp-error.test	2007-09-13 13:13:10 -03:00
> @@ -2090,6 +2090,26 @@ end//
>  delimiter ;//
>  
>  #
> +# Bug#29816 Syntactically wrong query fails with misleading error message
> +#
> +
> +--disable_warnings
> +DROP DATABASE IF EXISTS mysqltest;
> +--enable_warnings
> +CREATE DATABASE mysqltest;
> +USE mysqltest;
> +DROP DATABASE mysqltest;
> +--error ER_PARSE_ERROR
> +SELECT inexistent(), 1 + ,;
> +--error ER_SP_DOES_NOT_EXIST
> +SELECT inexistent();
> +--error ER_PARSE_ERROR
> +SELECT .inexistent();
> +--error ER_PARSE_ERROR
> +SELECT ..inexistent();
> +USE test;
> +
> +#
>  # BUG#NNNN: New bug synopsis
>  #
>  #--disable_warnings
> diff -Nrup a/mysql-test/t/udf.test b/mysql-test/t/udf.test
> --- a/mysql-test/t/udf.test	2007-06-18 18:55:07 -03:00
> +++ b/mysql-test/t/udf.test	2007-09-13 13:13:10 -03:00
> @@ -311,5 +311,19 @@ drop table t1;
>  drop function metaphon;
>  set GLOBAL query_cache_size=default;
>  
> +#
> +# Bug#28318  CREATE FUNCTION (UDF) requires a schema
> +#
> +
> +--disable_warnings
> +DROP DATABASE IF EXISTS mysqltest;
> +--enable_warnings
> +CREATE DATABASE mysqltest;
> +USE mysqltest;
> +DROP DATABASE mysqltest;
> +--replace_result $UDF_EXAMPLE_LIB UDF_EXAMPLE_LIB
> +eval CREATE FUNCTION metaphon RETURNS STRING SONAME "$UDF_EXAMPLE_LIB";
> +DROP FUNCTION metaphon;
> +USE test;
>  
>  --echo End of 5.0 tests.
> diff -Nrup a/sql/sp.cc b/sql/sp.cc
> --- a/sql/sp.cc	2007-07-25 11:38:48 -03:00
> +++ b/sql/sp.cc	2007-09-13 13:13:11 -03:00
> @@ -1405,12 +1405,12 @@ static bool add_used_routine(LEX *lex, Q
>    {
>      Sroutine_hash_entry *rn=
>        (Sroutine_hash_entry *)arena->alloc(sizeof(Sroutine_hash_entry) +
> -                                          key->length);
> +                                          key->length + 1);
>      if (!rn)              // OOM. Error will be reported using fatal_error().
>        return FALSE;
>      rn->key.length= key->length;
>      rn->key.str= (char *)rn + sizeof(Sroutine_hash_entry);
> -    memcpy(rn->key.str, key->str, key->length);
> +    memcpy(rn->key.str, key->str, key->length + 1);
>      my_hash_insert(&lex->sroutines, (byte *)rn);
>      lex->sroutines_list.link_in_list((byte *)rn, (byte **)&rn->next);
>      rn->belong_to_view= belong_to_view;
> @@ -1595,7 +1595,7 @@ sp_cache_routines_and_add_tables_aux(THD
>  
>    for (Sroutine_hash_entry *rt= start; rt; rt= rt->next)
>    {
> -    sp_name name(rt->key.str, rt->key.length);
> +    sp_name name(thd, rt->key.str, rt->key.length);
>      int type= rt->key.str[0];
>      sp_head *sp;
>  
> @@ -1603,13 +1603,6 @@ sp_cache_routines_and_add_tables_aux(THD
>                                &thd->sp_func_cache :
> &thd->sp_proc_cache),
>                                &name)))
>      {
> -      name.m_name.str= strchr(name.m_qname.str, '.');
> -      name.m_db.length= name.m_name.str - name.m_qname.str;
> -      name.m_db.str= strmake_root(thd->mem_root, name.m_qname.str,
> -                                  name.m_db.length);
> -      name.m_name.str+= 1;
> -      name.m_name.length= name.m_qname.length - name.m_db.length - 1;
> -
>        switch ((ret= db_find_routine(thd, type, &name, &sp)))
>        {
>        case SP_OK:
> diff -Nrup a/sql/sp_head.cc b/sql/sp_head.cc
> --- a/sql/sp_head.cc	2007-07-31 09:23:23 -03:00
> +++ b/sql/sp_head.cc	2007-09-13 13:13:11 -03:00
> @@ -369,17 +369,42 @@ sp_eval_expr(THD *thd, Field *result_fie
>   *
>   */
>  
> +sp_name::sp_name(THD *thd, char *key, uint key_len)
> +{
> +  m_sroutines_key.str= key;
> +  m_sroutines_key.length= key_len;
> +  m_qname.str= ++key;
> +  m_qname.length= key_len - 1;
> +  if ((m_name.str= strchr(m_qname.str, '.')))
> +  {
> +    m_db.length= m_name.str - key;
> +    m_db.str= strmake_root(thd->mem_root, key, m_db.length);
> +    m_name.str++;
> +    m_name.length= m_qname.length - m_db.length - 1;
> +  }
> +  else
> +  {
> +    m_name.str= m_qname.str;
> +    m_name.length= m_qname.length;
> +    m_db.str= 0;
> +    m_db.length= 0;
> +  }
> +  m_explicit_name= false;
> +}
> +
>  void
>  sp_name::init_qname(THD *thd)
>  {
> -  m_sroutines_key.length=  m_db.length + m_name.length + 2;
> +  const uint dot= !!m_db.length;
> +  /* m_sroutines format: m_type + [database + dot] + name + nul */
> +  m_sroutines_key.length= 1 + m_db.length + dot + m_name.length;
>    if (!(m_sroutines_key.str= thd->alloc(m_sroutines_key.length + 1)))
>      return;
>    m_qname.length= m_sroutines_key.length - 1;
>    m_qname.str= m_sroutines_key.str + 1;
> -  sprintf(m_qname.str, "%.*s.%.*s",
> +  sprintf(m_qname.str, "%.*s%.*s%.*s",
>  	  m_db.length, (m_db.length ? m_db.str : ""),
> -	  m_name.length, m_name.str);
> +          dot, ".", m_name.length, m_name.str);
>  }
>  
>  
> diff -Nrup a/sql/sp_head.h b/sql/sp_head.h
> --- a/sql/sp_head.h	2007-07-12 15:26:38 -03:00
> +++ b/sql/sp_head.h	2007-09-13 13:13:11 -03:00
> @@ -72,16 +72,7 @@ public:
>      Creates temporary sp_name object from key, used mainly
>      for SP-cache lookups.
>    */
> -  sp_name(char *key, uint key_len)
> -  {
> -    m_sroutines_key.str= key;
> -    m_sroutines_key.length= key_len;
> -    m_name.str= m_qname.str= key + 1;
> -    m_name.length= m_qname.length= key_len - 1;
> -    m_db.str= 0;
> -    m_db.length= 0;
> -    m_explicit_name= false;
> -  }
> +  sp_name(THD *thd, char *key, uint key_len);
>  
>    // Init. the qualified name from the db and name.
>    void init_qname(THD *thd);	// thd for memroot allocation
> diff -Nrup a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> --- a/sql/sql_yacc.yy	2007-08-28 14:16:02 -03:00
> +++ b/sql/sql_yacc.yy	2007-09-13 13:13:11 -03:00
> @@ -1569,13 +1569,15 @@ sp_name:
>  	| ident
>  	  {
>              LEX *lex= Lex;
> -            LEX_STRING db;
> +            LEX_STRING db= {0,0};
> +            THD *thd= YYTHD;
> +
>  	    if (check_routine_name($1))
>              {
>  	      my_error(ER_SP_WRONG_NAME, MYF(0), $1.str);
>  	      MYSQL_YYABORT;
>  	    }
> -            if (lex->copy_db_to(&db.str, &db.length))
> +            if (thd->db && lex->copy_db_to(&db.str,
> &db.length))
>   
Or your suggestion on IRC, using:
thd->copy_db_to()

>                MYSQL_YYABORT;
>  	    $$= new sp_name(db, $1, false);
>              if ($$)
> @@ -1625,6 +1627,13 @@ create_function_tail:
>  	      my_error(ER_SP_NO_RECURSIVE_CREATE, MYF(0), "FUNCTION");
>  	      MYSQL_YYABORT;
>  	    }
> +
> +            if (!lex->spname->m_db.str || !lex->spname->m_db.length)
>   
Please only test only one of m_db.str or m_db.length
In general, we trust the internal integrity of the server,
and don't write defensive code.


> +            {
> +              my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0));
> +              MYSQL_YYABORT;
> +            }
> +
>  	    /* Order is important here: new - reset - init */
>  	    sp= new sp_head();
>  	    sp->reset_thd_mem_root(thd);
> @@ -5194,8 +5203,8 @@ simple_expr:
>  #endif /* HAVE_DLOPEN */
>              {
>                THD *thd= lex->thd;
> -              LEX_STRING db;
> -              if (lex->copy_db_to(&db.str, &db.length))
> +              LEX_STRING db= {0,0};
> +              if (thd->db && lex->copy_db_to(&db.str,
> &db.length))
>   
Same as earlier comment

>                  MYSQL_YYABORT;
>                sp_name *name= new sp_name(db, $1, false);
>                if (name)
> @@ -9730,7 +9739,13 @@ trigger_tail:
>  	    my_error(ER_SP_NO_RECURSIVE_CREATE, MYF(0), "TRIGGER");
>  	    MYSQL_YYABORT;
>  	  }
> -	
> +
> +	  if (!$3->m_db.str || !$3->m_db.length)
>   
Same as earlier comment

> +	  {
> +	    my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0));
> +	    MYSQL_YYABORT;
> +	  }
> +
>  	  if (!(sp= new sp_head()))
>  	    MYSQL_YYABORT;
>  	  sp->reset_thd_mem_root(thd);
> @@ -9810,6 +9825,12 @@ sp_tail:
>  	  if (lex->sphead)
>  	  {
>  	    my_error(ER_SP_NO_RECURSIVE_CREATE, MYF(0), "PROCEDURE");
> +	    MYSQL_YYABORT;
> +	  }
> +
> +	  if (!$3->m_db.str || !$3->m_db.length)
>   
And here too
> +	  {
> +	    my_message(ER_NO_DB_ERROR, ER(ER_NO_DB_ERROR), MYF(0));
>  	    MYSQL_YYABORT;
>  	  }
>  
>
>