Bug#27430 "Crash in subquery code when in PS and table DDL changed after

PREPARE": rename members, methods, classes to follow the spec 
(a code review request)


sql/mysql_priv.h:
  enum_metadata_type -> enum_table_ref_type
sql/sp_head.cc:
  Metadata_version_observer -> Reprepare_observer
sql/sql_base.cc:
  metadata -> table_ref
sql/sql_class.cc:
  Replace an abstract interface with a concrete implementation.
sql/sql_class.h:
  enum_metadata_type -> enum_table_ref_type
sql/sql_prepare.cc:
  Move implementation of Execute_observer to sql_class.cc and
  rename the class to Reprepare_observer.
  Use getters instead of direct access to the members.
sql/table.h:
  metadata -> table_ref

sql/mysql_priv.h

@@ -700,14 +700,14 @@ const char *set_thd_proc_info(THD *thd, const char *info,
   @sa Prepared_statement::reprepare()
 */
 
-enum enum_metadata_type
+enum enum_table_ref_type
 {
   /** Initial value set by the parser */
-  METADATA_NULL= 0,
-  METADATA_VIEW,
-  METADATA_BASE_TABLE,
-  METADATA_I_S_TABLE,
-  METADATA_TMP_TABLE
+  TABLE_REF_NULL= 0,
+  TABLE_REF_VIEW,
+  TABLE_REF_BASE_TABLE,
+  TABLE_REF_I_S_TABLE,
+  TABLE_REF_TMP_TABLE
 };
 
 /*

sql/sp_head.cc

@@ -1068,7 +1068,7 @@ sp_head::execute(THD *thd)
   LEX *old_lex;
   Item_change_list old_change_list;
   String old_packet;
-  Metadata_version_observer *save_metadata_observer= thd->m_metadata_observer;
+  Reprepare_observer *save_reprepare_observer= thd->m_reprepare_observer;
 
   Object_creation_ctx *saved_creation_ctx;
 
@@ -1154,7 +1154,7 @@ sp_head::execute(THD *thd)
     of substatements (Bug#12257, Bug#27011, Bug#32868, Bug#33000),
     but it's not implemented yet.
   */
-  thd->m_metadata_observer= 0;
+  thd->m_reprepare_observer= 0;
 
   /*
     It is also more efficient to save/restore current thd->lex once when
@@ -1317,7 +1317,7 @@ sp_head::execute(THD *thd)
   thd->derived_tables= old_derived_tables;
   thd->variables.sql_mode= save_sql_mode;
   thd->abort_on_warning= save_abort_on_warning;
-  thd->m_metadata_observer= save_metadata_observer;
+  thd->m_reprepare_observer= save_reprepare_observer;
 
   thd->stmt_arena= old_arena;
   state= EXECUTED;

sql/sql_base.cc

@@ -3737,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_id_equal()
-  @sa TABLE_SHARE::get_metadata_id()
+  @sa TABLE_LIST::is_table_ref_id_equal()
+  @sa TABLE_SHARE::get_table_ref_id()
 
   @param[in]      thd         used to report errors
   @param[in,out]  tables      TABLE_LIST instance created by the parser
@@ -3754,10 +3754,10 @@ bool
 check_and_update_table_version(THD *thd,
                                TABLE_LIST *tables, TABLE_SHARE *table_share)
 {
-  if (! tables->is_metadata_id_equal(table_share))
+  if (! tables->is_table_ref_id_equal(table_share))
   {
-    if (thd->m_metadata_observer &&
-        thd->m_metadata_observer->report_error(thd))
+    if (thd->m_reprepare_observer &&
+        thd->m_reprepare_observer->report_error(thd))
     {
       /*
         Version of the table share is different from the
@@ -3768,14 +3768,14 @@ check_and_update_table_version(THD *thd,
       return TRUE;
     }
     /* Always maintain the latest version and type */
-    tables->set_metadata_id(table_share);
+    tables->set_table_ref_id(table_share);
   }
 #if 0
 #ifndef DBUG_OFF
   /* Spuriously reprepare each statement. */
-  if (thd->m_metadata_observer && thd->stmt_arena->is_reprepared == FALSE)
+  if (thd->m_reprepare_observer && thd->stmt_arena->is_reprepared == FALSE)
   {
-    thd->m_metadata_observer->report_error(thd);
+    thd->m_reprepare_observer->report_error(thd);
     return TRUE;
   }
 #endif
@@ -3865,7 +3865,7 @@ static int open_unireg_entry(THD *thd, TABLE *entry, TABLE_LIST *table_list,
       Note, the assert below is known to fail inside stored
       procedures (Bug#27011).
     */
-    DBUG_ASSERT(thd->m_metadata_observer);
+    DBUG_ASSERT(thd->m_reprepare_observer);
     check_and_update_table_version(thd, table_list, share);
     /* Always an error. */
     DBUG_ASSERT(thd->is_error());

sql/sql_class.cc

@@ -198,6 +198,19 @@ bool foreign_key_prefix(Key *a, Key *b)
 ** Thread specific functions
 ****************************************************************************/
 
+/** Push an error to the error stack and return TRUE for now. */
+
+bool
+Reprepare_observer::report_error(THD *thd)
+{
+  my_error(ER_NEED_REPREPARE, MYF(ME_NO_WARNING_FOR_ERROR|ME_NO_SP_HANDLER));
+
+  m_invalidated= TRUE;
+
+  return TRUE;
+}
+
+
 Open_tables_state::Open_tables_state(ulong version_arg)
   :version(version_arg), state_flags(0U)
 {
@@ -360,10 +373,6 @@ char *thd_security_context(THD *thd, char *buffer, unsigned int length,
   return thd->strmake(str.ptr(), str.length());
 }
 
-Metadata_version_observer::~Metadata_version_observer()
-{
-}
-
 /**
   Clear this diagnostics area. 
 
@@ -2774,7 +2783,7 @@ void THD::restore_backup_open_tables_state(Open_tables_state *backup)
               handler_tables == 0 && derived_tables == 0 &&
               lock == 0 && locked_tables == 0 &&
               prelocked_mode == NON_PRELOCKED &&
-              m_metadata_observer == NULL);
+              m_reprepare_observer == NULL);
   set_open_tables_state(backup);
   DBUG_VOID_RETURN;
 }

sql/sql_class.h

@@ -24,7 +24,7 @@
 #include "rpl_tblmap.h"
 
 /**
-  An abstract interface that can be used to take an action when
+  An interface that is used to take an action when
   the locking module notices that a table version has changed
   since the last execution. "Table" here may refer to any kind of
   table -- a base table, a temporary table, a view or an
@@ -36,36 +36,35 @@
   parse tree *may* be no longer valid, e.g. in case it contains
   optimizations that depend on table metadata.
 
-  This class provides an abstract interface (a method) that is
+  This class provides an interface (a method) that is
   invoked when such a situation takes place.
-  The implementation of the interface in most cases simply
-  reports an error, but the exact details depend on the nature of
-  the SQL statement.
+  The implementation of the method simply reports an error, but
+  the exact details depend on the nature of the SQL statement.
 
   At most 1 instance of this class is active at a time, in which
-  case THD::m_metadata_observer is not NULL.
+  case THD::m_reprepare_observer is not NULL.
 
   @sa check_and_update_table_version() for details of the
   version tracking algorithm 
 
-  @sa Execute_observer for details of how we detect that
-  a metadata change is fatal and a re-prepare is necessary
-
-  @sa Open_tables_state::m_metadata_observer for the life cycle
+  @sa Open_tables_state::m_reprepare_observer for the life cycle
   of metadata observers.
 */
 
-class Metadata_version_observer
+class Reprepare_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 report_error(THD *thd)= 0;
+  bool report_error(THD *thd);
+  bool is_invalidated() const { return m_invalidated; }
+  void reset_reprepare_observer() { m_invalidated= FALSE; }
+private:
+  bool m_invalidated;
 };
 
 
@@ -848,7 +847,7 @@ class Open_tables_state
     tracking.
     @sa check_and_update_table_version()
   */
-  Metadata_version_observer *m_metadata_observer;
+  Reprepare_observer *m_reprepare_observer;
 
   /**
     List of regular tables in use by this thread. Contains temporary and
@@ -953,7 +952,7 @@ class Open_tables_state
     extra_lock= lock= locked_tables= 0;
     prelocked_mode= NON_PRELOCKED;
     state_flags= 0U;
-    m_metadata_observer= NULL;
+    m_reprepare_observer= NULL;
   }
 };
 

sql/sql_prepare.cc

@@ -116,37 +116,6 @@ class Select_fetch_protocol_binary: public select_send
 #endif
 };
 
-/**
-  If a metadata changed, report a respective error to trigger
-  re-prepare of a prepared statement.
-*/
-
-class Execute_observer: public Metadata_version_observer
-{
-public:
-  virtual bool report_error(THD *thd);
-  /** Set to TRUE if metadata of some used table has changed since prepare */
-  bool m_invalidated;
-};
-
-/**
-  Push an error to the error stack and return TRUE for now.
-  In future we must take special care of statements like CREATE
-  TABLE ... SELECT.  Should we re-prepare such statements every
-  time?
-*/
-
-bool
-Execute_observer::report_error(THD *thd)
-{
-  DBUG_ENTER("Execute_observer::report_error");
-
-  my_error(ER_NEED_REPREPARE, MYF(ME_NO_WARNING_FOR_ERROR|ME_NO_SP_HANDLER));
-
-  m_invalidated= TRUE;
-  DBUG_RETURN(TRUE);
-}
-
 /****************************************************************************/
 
 /**
@@ -3219,7 +3188,7 @@ Prepared_statement::execute_loop(String *expanded_query,
                                  uchar *packet_end)
 {
   const int MAX_REPREPARE_ATTEMPTS= 3;
-  Execute_observer execute_observer;
+  Reprepare_observer reprepare_observer;
   bool error;
   int reprepare_attempt= 0;
 
@@ -3227,7 +3196,7 @@ Prepared_statement::execute_loop(String *expanded_query,
     return TRUE;
 
 reexecute:
-  execute_observer.m_invalidated= FALSE;
+  reprepare_observer.reset_reprepare_observer();
 
   /*
     If the free_list is not empty, we'll wrongly free some externally
@@ -3245,8 +3214,8 @@ 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;
+    DBUG_ASSERT(thd->m_reprepare_observer == NULL);
+    thd->m_reprepare_observer = &reprepare_observer;
   }
 
   if (!(specialflag & SPECIAL_NO_PRIOR))
@@ -3257,10 +3226,10 @@ Prepared_statement::execute_loop(String *expanded_query,
   if (!(specialflag & SPECIAL_NO_PRIOR))
     my_pthread_setprio(pthread_self(), WAIT_PRIOR);
 
-  thd->m_metadata_observer= NULL;
+  thd->m_reprepare_observer= NULL;
 
   if (error && !thd->is_fatal_error && !thd->killed &&
-      execute_observer.m_invalidated &&
+      reprepare_observer.is_invalidated() &&
       reprepare_attempt++ < MAX_REPREPARE_ATTEMPTS)
   {
     DBUG_ASSERT(thd->main_da.sql_errno() == ER_NEED_REPREPARE);

sql/table.h

@@ -440,21 +440,21 @@ typedef struct st_table_share
 
   /**
     Convert unrelated members of TABLE_SHARE to one enum
-    representing its metadata type.
+    representing its type.
 
     @todo perhaps we need to have a member instead of a function.
   */
-  enum enum_metadata_type get_metadata_type() const
+  enum enum_table_ref_type get_table_ref_type() const
   {
     if (is_view)
-      return METADATA_VIEW;
+      return TABLE_REF_VIEW;
     switch (tmp_table) {
     case NO_TMP_TABLE:
-      return METADATA_BASE_TABLE;
+      return TABLE_REF_BASE_TABLE;
     case SYSTEM_TMP_TABLE:
-      return METADATA_I_S_TABLE;
+      return TABLE_REF_I_S_TABLE;
     default:
-      return METADATA_TMP_TABLE;
+      return TABLE_REF_TMP_TABLE;
     }
   }
   /**
@@ -479,7 +479,7 @@ typedef struct st_table_share
    to validate prepared statements.
 
    Firstly, sets (in mathematical sense) of version numbers
-   never intersect for different metadata types. Therefore,
+   never intersect for different table types. Therefore,
    version id of a temporary table is never compared with
    a version id of a view, and vice versa.
 
@@ -525,14 +525,14 @@ typedef struct st_table_share
    When this is done, views will be handled in the same fashion
    as the base tables.
 
-   Finally, by taking into account metadata type, we always
+   Finally, by taking into account table type, we always
    track that a change has taken place when a view is replaced
    with a base table, a base table is replaced with a temporary
    table and so on.
 
-   @sa TABLE_LIST::is_metadata_id_equal()
+   @sa TABLE_LIST::is_table_ref_id_equal()
   */
-  ulong get_metadata_version() const
+  ulong get_table_ref_version() const
   {
     return (tmp_table == SYSTEM_TMP_TABLE || is_view) ? 0 : table_map_id;
   }
@@ -1340,10 +1340,10 @@ struct TABLE_LIST
     @sa check_and_update_table_version()
   */
   inline
-  bool is_metadata_id_equal(TABLE_SHARE *s) const
+  bool is_table_ref_id_equal(TABLE_SHARE *s) const
   {
-    return (m_metadata_type == s->get_metadata_type() &&
-            m_metadata_version == s->get_metadata_version());
+    return (m_table_ref_type == s->get_table_ref_type() &&
+            m_table_ref_version == s->get_table_ref_version());
   }
 
   /**
@@ -1353,10 +1353,10 @@ struct TABLE_LIST
     @sa check_and_update_table_version()
   */
   inline
-  void set_metadata_id(TABLE_SHARE *s)
+  void set_table_ref_id(TABLE_SHARE *s)
   {
-    m_metadata_type= s->get_metadata_type();
-    m_metadata_version= s->get_metadata_version();
+    m_table_ref_type= s->get_table_ref_type();
+    m_table_ref_version= s->get_table_ref_version();
   }
 
 private:
@@ -1370,9 +1370,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_id() */
-  enum enum_metadata_type m_metadata_type;
+  enum enum_table_ref_type m_table_ref_type;
   /** See comments for set_metadata_id() */
-  ulong m_metadata_version;
+  ulong m_table_ref_version;
 };
 
 class Item;