Cleanup-patch for BUG#25843: changing default database between

PREPARE and EXECUTE of statement breaks binlog.


sql/sp.cc:
  - Polishing sp_use_new_db():
    - renamed no_access_check to force_switch to be more adequate;
    - fixed comment;
sql/sql_class.h:
  Polishing: fixed comment.
sql/sql_db.cc:
  1. Use mysql_change_db_impl() to reset current database instead of
  THD::set_db() in mysql_rm_db(). THD::set_db() does not take care of
  THD::db_access and database attributes (@@collation_database).
  2. Polishing: add, fix comments.

sql/sp.cc

@@ -574,7 +574,7 @@ db_load_routine(THD *thd, int type, sp_name *name, sp_head **sphp,
     we'll update it later in switch_query_ctx().
   */
 
-  if ((ret= sp_use_new_db(thd, name->m_db, &old_db, 1, &dbchanged)))
+  if ((ret= sp_use_new_db(thd, name->m_db, &old_db, TRUE, &dbchanged)))
     goto end;
 
   thd->spcont= NULL;
@@ -2027,34 +2027,41 @@ create_string(THD *thd, String *buf,
 
 
 
-/*
+/**
   Change the current database if needed.
 
-  SYNOPSIS
-    sp_use_new_db()
-      thd            thread handle
-      new_db         new database name (a string and its length)
-      old_db         [IN] str points to a buffer where to store the old
-                          database, length contains the size of the buffer
-                     [OUT] if old db was not NULL, its name is copied
-                     to the buffer pointed at by str and length is updated
-                     accordingly. Otherwise str[0] is set to '\0' and length
-                     is set to 0. The out parameter should be used only if
-                     the database name has been changed (see dbchangedp).
-     dbchangedp      [OUT] is set to TRUE if the current database is changed,
-                     FALSE otherwise. A database is not changed if the old
-                     name is the same as the new one, both names are empty,
-                     or an error has occurred.
-
-  RETURN VALUE
-    0                success
-    1                access denied or out of memory (the error message is
-                     set in THD)
+  @param[in]      thd             thread handle
+  @param[in]      new_db          new database name
+  @param[in, out] old_db          IN: str points to a buffer where to store
+                                  the old database, length contains the
+                                  size of the buffer
+                                  OUT: if old db was not NULL, its name is
+                                  copied to the buffer pointed at by str
+                                  and length is updated accordingly.
+                                  Otherwise str[0] is set to '\0' and
+                                  length is set to 0. The out parameter
+                                  should be used only if the database name
+                                  has been changed (see dbchangedp).
+  @param[in]      force_switch    Flag to mysql_change_db(). For more information,
+                                  see mysql_change_db() comment.
+  @param[out]     dbchangedp      is set to TRUE if the current database is
+                                  changed, FALSE otherwise. The current
+                                  database is not changed if the old name
+                                  is equal to the new one, both names are
+                                  empty, or an error has occurred.
+
+  @return Operation status.
+    @retval 0 on success
+    @retval 1 access denied or out of memory
+              (the error message is set in THD)
 */
 
 int
-sp_use_new_db(THD *thd, LEX_STRING new_db, LEX_STRING *old_db,
-	      bool no_access_check, bool *dbchangedp)
+sp_use_new_db(THD *thd,
+              LEX_STRING new_db,
+              LEX_STRING *old_db,
+              bool force_switch,
+              bool *dbchangedp)
 {
   int ret;
   DBUG_ENTER("sp_use_new_db");
@@ -2085,7 +2092,7 @@ sp_use_new_db(THD *thd, LEX_STRING new_db, LEX_STRING *old_db,
     DBUG_RETURN(0);
   }
 
-  ret= mysql_change_db(thd, &new_db, no_access_check);
+  ret= mysql_change_db(thd, &new_db, force_switch);
 
   *dbchangedp= ret == 0;
   DBUG_RETURN(ret);

sql/sql_class.h

@@ -1802,11 +1802,27 @@ public:
     }
   }
 
-  /*
-    Initialize the current database from a NULL-terminated string with length
-    If we run out of memory, we free the current database and return TRUE.
-    This way the user will notice the error as there will be no current
-    database selected (in addition to the error message set by malloc).
+  /**
+    Set the current database; use deep copy of C-string.
+
+    @param new_db     a pointer to the new database name.
+    @param new_db_len length of the new database name.
+
+    Initialize the current database from a NULL-terminated string with
+    length. If we run out of memory, we free the current database and
+    return TRUE.  This way the user will notice the error as there will be
+    no current database selected (in addition to the error message set by
+    malloc).
+
+    @note This operation just sets {thd->db, thd->db_length}. Switching the
+    current database usually involves other actions, like switching other
+    database attributes including security context. In the future, this
+    operation will be made private and more convenient interface will be
+    provided.
+
+    @return Operation status
+      @retval FALSE Success
+      @retval TRUE  Out-of-memory error
   */
   bool set_db(const char *new_db, size_t new_db_len)
   {
@@ -1821,6 +1837,19 @@ public:
     db_length= db ? new_db_len : 0;
     return new_db && !db;
   }
+
+  /**
+    Set the current database; use shallow copy of C-string.
+
+    @param new_db     a pointer to the new database name.
+    @param new_db_len length of the new database name.
+
+    @note This operation just sets {thd->db, thd->db_length}. Switching the
+    current database usually involves other actions, like switching other
+    database attributes including security context. In the future, this
+    operation will be made private and more convenient interface will be
+    provided.
+  */
   void reset_db(char *new_db, size_t new_db_len)
   {
     db= new_db;

sql/sql_db.cc

@@ -39,6 +39,10 @@ static long mysql_rm_known_files(THD *thd, MY_DIR *dirp,
          
 static long mysql_rm_arc_files(THD *thd, MY_DIR *dirp, const char *org_path);
 static my_bool rm_dir_w_symlink(const char *org_path, my_bool send_error);
+static void mysql_change_db_impl(THD *thd,
+                                 LEX_STRING *new_db_name,
+                                 ulong new_db_access,
+                                 CHARSET_INFO *new_db_charset);
 
 
 /* Database lock hash */
@@ -988,7 +992,7 @@ exit:
     it to 0.
   */
   if (thd->db && !strcmp(thd->db, db))
-    thd->set_db(NULL, 0);
+    mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);
   VOID(pthread_mutex_unlock(&LOCK_mysql_create_db));
   start_waiting_global_read_lock(thd);
 exit2:
@@ -1347,21 +1351,48 @@ static void mysql_change_db_impl(THD *thd,
 
 
 /**
-  @brief Change the current database.
+  @brief Change the current database and its attributes.
 
   @param thd          thread handle
   @param new_db_name  database name
-  @param force_switch if this flag is set (TRUE), mysql_change_db() will
-                      switch to NULL db if the specified database is not
-                      available anymore. Corresponding warning will be
-                      thrown in this case. This flag is used to change
-                      database in stored-routine-execution code.
-
-  @details Check that the database name corresponds to a valid and existent
-  database, check access rights (unless called with no_access_check), and
-  set the current database. This function is called to change the current
-  database upon user request (COM_CHANGE_DB command) or temporarily, to
-  execute a stored routine.
+  @param force_switch if force_switch is FALSE, then the operation will fail if
+
+                        - new_db_name is NULL or empty;
+
+                        - OR new database name is invalid
+                          (check_db_name() failed);
+
+                        - OR user has no privilege on the new database;
+
+                        - OR new database does not exist;
+
+                      if force_switch is TRUE, then
+
+                        - if new_db_name is NULL or empty, the current
+                          database will be NULL, @@collation_database will
+                          be set to @@collation_server, the operation will
+                          succeed.
+
+                        - if new database name is invalid
+                          (check_db_name() failed), the current database
+                          will be NULL, @@collation_database will be set to
+                          @@collation_server, but the operation will fail;
+
+                        - user privileges will not be checked
+                          (THD::db_access however is updated);
+
+                          TODO: is this really the intention?
+                                (see sp-security.test).
+
+                        - if new database does not exist,the current database
+                          will be NULL, @@collation_database will be set to
+                          @@collation_server, a warning will be thrown, the
+                          operation will succeed.
+
+  @details The function checks that the database name corresponds to a
+  valid and existent database, checks access rights and changes the current
+  database with database attributes (@@collation_database session variable,
+  THD::db_access).
 
   This function is not the only way to switch the database that is
   currently employed. When the replication slave thread switches the
@@ -1398,8 +1429,13 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)
     if (force_switch)
     {
       /*
-        This can only happen when we restore the old db in THD after
-        execution of a routine is complete. Change db to NULL.
+        This can happen only if we're switching the current database back
+        after loading stored program. The thing is that loading of stored
+        program can happen when there is no current database.
+
+        TODO: actually, new_db_name and new_db_name->str seem to be always
+        non-NULL. In case of stored program, new_db_name->str == "" and
+        new_db_name->length == 0.
       */
 
       mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);
@@ -1417,7 +1453,7 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)
   if (my_strcasecmp(system_charset_info, new_db_name->str,
                     INFORMATION_SCHEMA_NAME.str) == 0)
   {
-    /* Switch database to INFORMATION_SCHEMA. */
+    /* Switch the current database to INFORMATION_SCHEMA. */
 
     mysql_change_db_impl(thd, &INFORMATION_SCHEMA_NAME, SELECT_ACL,
                          system_charset_info);
@@ -1444,8 +1480,8 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)
     even if we are called from sp_head::execute().
 
     It's next to impossible however to get this error when we are called
-    from sp_head::execute(). But let's switch database to NULL in this case
-    to be sure.
+    from sp_head::execute(). But let's switch the current database to NULL
+    in this case to be sure.
   */
 
   if (check_db_name(&new_db_file_name))
@@ -1454,10 +1490,8 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)
     my_free(new_db_file_name.str, MYF(0));
 
     if (force_switch)
-    {
-      /* Change db to NULL. */
       mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);
-    }
+
     DBUG_RETURN(TRUE);
   }
 
@@ -1492,6 +1526,8 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)
   {
     if (force_switch)
     {
+      /* Throw a warning and free new_db_file_name. */
+
       push_warning_printf(thd, MYSQL_ERROR::WARN_LEVEL_NOTE,
                           ER_BAD_DB_ERROR, ER(ER_BAD_DB_ERROR),
                           new_db_file_name.str);
@@ -1502,12 +1538,19 @@ bool mysql_change_db(THD *thd, const LEX_STRING *new_db_name, bool force_switch)
 
       mysql_change_db_impl(thd, NULL, 0, thd->variables.collation_server);
 
+      /* The operation succeed. */
+
       DBUG_RETURN(FALSE);
     }
     else
     {
+      /* Report an error and free new_db_file_name. */
+
       my_error(ER_BAD_DB_ERROR, MYF(0), new_db_file_name.str);
       my_free(new_db_file_name.str, MYF(0));
+
+      /* The operation failed. */
+
       DBUG_RETURN(TRUE);
     }
   }
@@ -1820,7 +1863,7 @@ bool mysql_rename_db(THD *thd, LEX_STRING *old_db, LEX_STRING *new_db)
 
   /* Step9: Let's do "use newdb" if we renamed the current database */
   if (change_to_newdb)
-    error|= mysql_change_db(thd, new_db, 0);
+    error|= mysql_change_db(thd, new_db, FALSE);
 
 exit:
   pthread_mutex_lock(&LOCK_lock_db);