Implement some code review fixes for the fix for Bug#27430

"Crash in subquery code when in PS and table DDL changed after PREPARE"

include/my_sys.h

@@ -90,6 +90,9 @@ extern int NEAR my_errno;		/* Last error in mysys */
 #define ME_COLOUR1	((1 << ME_HIGHBYTE))	/* Possibly error-colours */
 #define ME_COLOUR2	((2 << ME_HIGHBYTE))
 #define ME_COLOUR3	((3 << ME_HIGHBYTE))
+#define ME_FATALERROR   1024    /* Fatal statement error */
+#define ME_NO_WARNING_FOR_ERROR 2048 /* Don't push a warning for error */
+#define ME_NO_SP_HANDLER 4096 /* Don't call stored routine error handlers */
 
 	/* Bits in last argument to fn_format */
 #define MY_REPLACE_DIR		1	/* replace dir in name with 'dir' */

sql/item.cc

@@ -3165,15 +3165,19 @@ void Item_param::print(String *str, enum_query_type query_type)
   Preserve the original parameter types and values
   when re-preparing a prepared statement.
 
-  Copy parameter type information and conversion function
-  pointers from a parameter of the old statement to the
-  corresponding parameter of the new one.
+  @details Copy parameter type information and conversion
+  function pointers from a parameter of the old statement
+  to the corresponding parameter of the new one.
 
   Move parameter values from the old parameters to the new
   one. We simply "exchange" the values, which allows
   to save on allocation and character set conversion in
   case a parameter is a string or a blob/clob.
 
+  The old parameter gets the value of this one, which
+  ensures that all memory of this parameter is freed
+  correctly.
+
   @param[in]  src   parameter item of the original
                     prepared statement
 */
@@ -3187,7 +3191,7 @@ Item_param::set_param_type_and_swap_value(Item_param *src)
   item_type= src->item_type;
   item_result_type= src->item_result_type;
 
-  collation.set(src->collation.collation);
+  collation.set(src->collation);
   maybe_null= src->maybe_null;
   null_value= src->null_value;
   max_length= src->max_length;

sql/mysqld.cc

@@ -2843,6 +2843,7 @@ int my_message_sql(uint error, const char *str, myf MyFlags)
       by the stored procedures code.
     */
     if (thd->spcont &&
+        ! (MyFlags & ME_NO_SP_HANDLER) &&
         thd->spcont->handle_error(error, MYSQL_ERROR::WARN_LEVEL_ERROR, thd))
     {
       /*
@@ -2852,7 +2853,8 @@ int my_message_sql(uint error, const char *str, myf MyFlags)
       DBUG_RETURN(0);
     }
 
-    if (!thd->no_warnings_for_error)
+    if (!thd->no_warnings_for_error &&
+        !(MyFlags & ME_NO_WARNING_FOR_ERROR))
     {
       /*
         Suppress infinite recursion if there a memory allocation error

sql/sql_base.cc

@@ -3722,9 +3722,8 @@ void assign_new_table_id(TABLE_SHARE *share)
   Compare metadata versions of an element obtained from the table
   definition cache and its corresponding node in the parse tree.
 
-  If the new and the old values mismatch, invoke
+  @details If the new and the old values mismatch, invoke
   Metadata_version_observer.
-
   At prepared statement prepare, all TABLE_LIST version values are
   NULL and we always have a mismatch. But there is no observer set
   in THD, and therefore no error is reported. Instead, we update
@@ -3738,8 +3737,8 @@ void assign_new_table_id(TABLE_SHARE *share)
 
   @sa Execute_observer
   @sa check_prepared_statement() to see cases when an observer is installed
-  @sa TABLE_LIST::is_metadata_version_equal()
-  @sa TABLE_SHARE::get_metadata_version()
+  @sa TABLE_LIST::is_metadata_id_equal()
+  @sa TABLE_SHARE::get_metadata_id()
 
   @param[in]      thd         used to report errors
   @param[in,out]  tables      TABLE_LIST instance created by the parser
@@ -3755,27 +3754,28 @@ bool
 check_and_update_table_version(THD *thd,
                                TABLE_LIST *tables, TABLE_SHARE *table_share)
 {
-  if (! tables->is_metadata_version_equal(table_share))
+  if (! tables->is_metadata_id_equal(table_share))
   {
     if (thd->m_metadata_observer &&
-        thd->m_metadata_observer->check_metadata_change(thd))
+        thd->m_metadata_observer->report_error(thd))
     {
       /*
         Version of the table share is different from the
         previous execution of the prepared statement, and it is
         unacceptable for this SQLCOM. Error has been reported.
       */
+      DBUG_ASSERT(thd->is_error());
       return TRUE;
     }
-    /* Always maintain the latest version */
-    tables->set_metadata_version(table_share);
+    /* Always maintain the latest version and type */
+    tables->set_metadata_id(table_share);
   }
 #if 0
 #ifndef DBUG_OFF
   /* Spuriously reprepare each statement. */
   if (thd->m_metadata_observer && thd->stmt_arena->is_reprepared == FALSE)
   {
-    thd->m_metadata_observer->check_metadata_change(thd);
+    thd->m_metadata_observer->report_error(thd);
     return TRUE;
   }
 #endif
@@ -3828,6 +3828,10 @@ static int open_unireg_entry(THD *thd, TABLE *entry, TABLE_LIST *table_list,
 
   if (share->is_view)
   {
+    /*
+      This table is a view. Validate its metadata version: in particular,
+      that it was a view when the statement was prepared.
+    */
     if (check_and_update_table_version(thd, table_list, share))
       goto err;
     if (table_list->i_s_requested_object &  OPEN_TABLE_ONLY)
@@ -4622,6 +4626,7 @@ int open_tables(THD *thd, TABLE_LIST **start, uint *counter, uint flags)
     }
     tables->table->grant= tables->grant;
 
+    /* Check and update metadata version of a base table. */
     if (check_and_update_table_version(thd, tables, tables->table->s))
     {
       result= -1;

sql/sql_class.h

@@ -57,16 +57,15 @@
 
 class Metadata_version_observer
 {
-protected:
-  virtual ~Metadata_version_observer();
 public:
+  virtual ~Metadata_version_observer();
   /**
     Check if a change of metadata is OK. In future
     the signature of this method may be extended to accept the old
     and the new versions, but since currently the check is very
     simple, we only need the THD to report an error.
   */
-  virtual bool check_metadata_change(THD *thd)= 0;
+  virtual bool report_error(THD *thd)= 0;
 };
 
 
@@ -2842,6 +2841,19 @@ class select_dumpvar :public select_result_interceptor {
 #define CF_STATUS_COMMAND	4
 #define CF_SHOW_TABLE_COMMAND	8
 #define CF_WRITE_LOGS_COMMAND  16
+/**
+  Must be set for SQL statements that may contain
+  Item expressions and/or use joins and tables.
+  Indicates that the parse tree of such statement may
+  contain rule-based optimizations that depend on metadata
+  (i.e. number of columns in a table), and consequently
+  that the statement must be re-prepared whenever
+  referenced metadata changes. Must not be set for
+  statements that themselves change metadata, e.g. RENAME,
+  ALTER and other DDL, since otherwise will trigger constant
+  reprepare. Consequently, complex item expressions and
+  joins are currently prohibited in these statements.
+*/
 #define CF_REEXECUTION_FRAGILE 32
 
 /* Functions in sql_class.cc */

sql/sql_prepare.cc

@@ -124,7 +124,7 @@ class Select_fetch_protocol_binary: public select_send
 class Execute_observer: public Metadata_version_observer
 {
 public:
-  virtual bool check_metadata_change(THD *thd);
+  virtual bool report_error(THD *thd);
   /** Set to TRUE if metadata of some used table has changed since prepare */
   bool m_invalidated;
 };
@@ -137,14 +137,12 @@ class Execute_observer: public Metadata_version_observer
 */
 
 bool
-Execute_observer::check_metadata_change(THD *thd)
+Execute_observer::report_error(THD *thd)
 {
-  bool save_thd_no_warnings_for_error= thd->no_warnings_for_error;
-  DBUG_ENTER("Execute_observer::notify_about_metadata_change");
+  DBUG_ENTER("Execute_observer::report_error");
+
+  my_error(ER_NEED_REPREPARE, MYF(ME_NO_WARNING_FOR_ERROR|ME_NO_SP_HANDLER));
 
-  thd->no_warnings_for_error= TRUE;
-  my_error(ER_NEED_REPREPARE, MYF(0));
-  thd->no_warnings_for_error= save_thd_no_warnings_for_error;
   m_invalidated= TRUE;
   DBUG_RETURN(TRUE);
 }
@@ -984,7 +982,7 @@ static bool emb_insert_params_with_log(Prepared_statement *stmt,
 /**
   Setup data conversion routines using an array of parameter
   markers from the original prepared statement.
-  Move the parameter data of the original prepared
+  Swap the parameter data of the original prepared
   statement to the new one.
 
   Used only when we re-prepare a prepared statement.
@@ -2844,7 +2842,7 @@ Select_fetch_protocol_binary::send_data(List<Item> &fields)
 ****************************************************************************/
 
 Prepared_statement::Prepared_statement(THD *thd_arg, Protocol *protocol_arg)
-  :Statement(0, &main_mem_root,
+  :Statement(NULL, &main_mem_root,
              INITIALIZED, ++thd_arg->statement_id_counter),
   thd(thd_arg),
   result(thd_arg),
@@ -3153,38 +3151,36 @@ Prepared_statement::set_parameters(String *expanded_query,
                                    uchar *packet, uchar *packet_end)
 {
   bool is_sql_ps= packet == NULL;
+  bool res;
 
   if (is_sql_ps)
   {
     /* SQL prepared statement */
-    if (set_params_from_vars(this, thd->lex->prepared_stmt_params,
-                                   expanded_query))
-      goto set_params_data_err;
+    res= set_params_from_vars(this, thd->lex->prepared_stmt_params,
+                              expanded_query);
   }
   else if (param_count)
   {
 #ifndef EMBEDDED_LIBRARY
     uchar *null_array= packet;
-    if (setup_conversion_functions(this, &packet, packet_end) ||
-        set_params(this, null_array, packet, packet_end,
-                   expanded_query))
-      goto set_params_data_err;
+    res= (setup_conversion_functions(this, &packet, packet_end) ||
+          set_params(this, null_array, packet, packet_end, expanded_query));
 #else
-  /*
-    In embedded library we re-install conversion routines each time
-    we set params, and also we don't need to parse packet.
-    So we do it in one function.
-  */
-    if (set_params_data(this, expanded_query))
-      goto set_params_data_err;
+    /*
+      In embedded library we re-install conversion routines each time
+      we set parameters, and also we don't need to parse packet.
+      So we do it in one function.
+    */
+    res= set_params_data(this, expanded_query);
 #endif
   }
-  return FALSE;
-set_params_data_err:
-  my_error(ER_WRONG_ARGUMENTS, MYF(0),
-           is_sql_ps ? "EXECUTE" : "mysql_stmt_execute");
-  reset_stmt_params(this);
-  return TRUE;
+  if (res)
+  {
+    my_error(ER_WRONG_ARGUMENTS, MYF(0),
+             is_sql_ps ? "EXECUTE" : "mysql_stmt_execute");
+    reset_stmt_params(this);
+  }
+  return res;
 }
 
 
@@ -3249,6 +3245,7 @@ Prepared_statement::execute_loop(String *expanded_query,
   if (sql_command_flags[lex->sql_command] &
       CF_REEXECUTION_FRAGILE)
   {
+    DBUG_ASSERT(thd->m_metadata_observer == NULL);
     thd->m_metadata_observer= &execute_observer;
   }
 
@@ -3260,12 +3257,13 @@ Prepared_statement::execute_loop(String *expanded_query,
   if (!(specialflag & SPECIAL_NO_PRIOR))
     my_pthread_setprio(pthread_self(), WAIT_PRIOR);
 
-  thd->m_metadata_observer= 0;
+  thd->m_metadata_observer= NULL;
 
   if (error && !thd->is_fatal_error && !thd->killed &&
       execute_observer.m_invalidated &&
       reprepare_attempt++ < MAX_REPREPARE_ATTEMPTS)
   {
+    DBUG_ASSERT(thd->main_da.sql_errno() == ER_NEED_REPREPARE);
     thd->clear_error();
 
     error= reprepare();
@@ -3299,44 +3297,40 @@ Prepared_statement::reprepare()
   LEX_STRING saved_cur_db_name=
     { saved_cur_db_name_buf, sizeof(saved_cur_db_name_buf) };
   LEX_STRING stmt_db_name= { db, db_length };
-  Prepared_statement *copy;
   bool cur_db_changed;
-  bool error= TRUE;
-
-  status_var_increment(thd->status_var.com_stmt_reprepare);
+  bool error;
 
-  copy= new Prepared_statement(thd, &thd->protocol_text);
+  Prepared_statement copy(thd, &thd->protocol_text);
 
-  if (! copy)
-    return TRUE;
+  status_var_increment(thd->status_var.com_stmt_reprepare);
 
   if (mysql_opt_change_db(thd, &stmt_db_name, &saved_cur_db_name, TRUE,
                           &cur_db_changed))
-    goto end;
+    return TRUE;
 
-  error= (name.str && copy->set_name(&name) ||
-          copy->prepare(query, query_length) ||
-          validate_metadata(copy));
+  error= (name.str && copy.set_name(&name) ||
+          copy.prepare(query, query_length) ||
+          validate_metadata(&copy));
 
   if (cur_db_changed)
     mysql_change_db(thd, &saved_cur_db_name, TRUE);
 
   if (! error)
   {
-    swap_prepared_statement(copy);
-    swap_parameter_array(param_array, copy->param_array, param_count);
+    swap_prepared_statement(&copy);
+    swap_parameter_array(param_array, copy.param_array, param_count);
 #ifndef DBUG_OFF
     is_reprepared= TRUE;
 #endif
     /*
-      Clear possible warnigns during re-prepare, it has to be completely
+      Clear possible warnings during reprepare, it has to be completely
       transparent to the user. We use mysql_reset_errors() since
       there were no separate query id issued for re-prepare.
+      Sic: we can't simply silence warnings during reprepare, because if
+      it's failed, we need to return all the warnings to the user.
     */
     mysql_reset_errors(thd, TRUE);
   }
-end:
-  delete copy;
   return error;
 }
 

sql/table.h

@@ -478,10 +478,10 @@ typedef struct st_table_share
    Let's try to explain why and how this limited solution allows
    to validate prepared statements.
 
-   Firstly, spaces (in mathematical sense) of version numbers
+   Firstly, sets (in mathematical sense) of version numbers
    never intersect for different metadata types. Therefore,
    version id of a temporary table is never compared with
-   a version id of a view or a temporary table, and vice versa.
+   a version id of a view, and vice versa.
 
    Secondly, for base tables, we know that each DDL flushes the
    respective share from the TDC. This ensures that whenever
@@ -530,11 +530,11 @@ typedef struct st_table_share
    with a base table, a base table is replaced with a temporary
    table and so on.
 
-   @sa TABLE_LIST::is_metadata_version_equal()
+   @sa TABLE_LIST::is_metadata_id_equal()
   */
   ulong get_metadata_version() const
   {
-    return tmp_table == SYSTEM_TMP_TABLE || is_view ? 0 : table_map_id;
+    return (tmp_table == SYSTEM_TMP_TABLE || is_view) ? 0 : table_map_id;
   }
 
 } TABLE_SHARE;
@@ -1340,7 +1340,7 @@ struct TABLE_LIST
     @sa check_and_update_table_version()
   */
   inline
-  bool is_metadata_version_equal(TABLE_SHARE *s) const
+  bool is_metadata_id_equal(TABLE_SHARE *s) const
   {
     return (m_metadata_type == s->get_metadata_type() &&
             m_metadata_version == s->get_metadata_version());
@@ -1353,7 +1353,7 @@ struct TABLE_LIST
     @sa check_and_update_table_version()
   */
   inline
-  void set_metadata_version(TABLE_SHARE *s)
+  void set_metadata_id(TABLE_SHARE *s)
   {
     m_metadata_type= s->get_metadata_type();
     m_metadata_version= s->get_metadata_version();
@@ -1369,9 +1369,9 @@ struct TABLE_LIST
 
   /* Remembered MERGE child def version.  See top comment in ha_myisammrg.cc */
   ulong         child_def_version;
-  /** See comments for set_metadata_version() */
+  /** See comments for set_metadata_id() */
   enum enum_metadata_type m_metadata_type;
-  /** See comments for set_metadata_version() */
+  /** See comments for set_metadata_id() */
   ulong m_metadata_version;
 };
 

tests/mysql_client_test.c

@@ -17418,6 +17418,7 @@ static void test_wl4166()
 
   verify_param_count(stmt, 7);
 
+  bzero(my_bind, sizeof(my_bind));
   /* tinyint */
   my_bind[0].buffer_type= MYSQL_TYPE_TINY;
   my_bind[0].buffer= (void *)&tiny_data;