MySQL Lists are EOL. Please join:

List:Commits« Previous MessageNext Message »
From:Thava Alagu Date:November 6 2009 6:36am
Subject:bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888) Bug#47994
View as plain text  
#At file:///home/thava/mysql/repo/backup/

 2888 Thava Alagu	2009-11-06
      BUG#47994 : Interruption of BACKUP command reported multiple times.
      
      When backup session is interrupted at certain stages, multiple duplicate
      warning messages are generated.
      
      Added additional checks to catch interruption of the command earlier
      to stop the backup kernel from executing further queries.
      Removed some duplicate pushing of warnings. 
      modified:
        mysql-test/suite/backup/r/backup_errors.result
        mysql-test/suite/backup/t/backup_errors.test
        mysql-test/suite/backup_engines/r/backup_interruption.result
        sql/backup/backup_info.cc
        sql/backup/backup_kernel.h
        sql/backup/kernel.cc
        sql/backup/logger.h
        sql/share/errmsg-utf8.txt
        sql/share/errmsg.txt
        sql/si_logs.cc
        sql/si_logs.h
        sql/si_objects.cc
        sql/si_objects.h

=== modified file 'mysql-test/suite/backup/r/backup_errors.result'
--- a/mysql-test/suite/backup/r/backup_errors.result	2009-10-21 12:15:53 +0000
+++ b/mysql-test/suite/backup/r/backup_errors.result	2009-11-06 06:36:22 +0000
@@ -596,10 +596,17 @@ SET SESSION DEBUG='-d';
 # Test error handling by injecting errors into BACKUP
 #
 #
+# Error injection in user privileges checking.
+#
+SET SESSION DEBUG='+d,ER_BACKUP_USER_PRIV_CHECK';
+BACKUP DATABASE db1 TO 'failed1.bak' OVERWRITE;
+ERROR HY000: Failed to perform access privileges check for user 'root'.
+SET SESSION DEBUG='-d';
+#
 # Error injection in si_object.cc:get_database_stub
 #
 SET SESSION DEBUG='+d,siobj_get_db_stub';
-BACKUP DATABASE db1 TO 'failed1.bak';
+BACKUP DATABASE db1 TO 'failed1.bak' OVERWRITE;
 ERROR HY000: Failed to add database `db1` to the catalog
 SET SESSION DEBUG='-d';
 #

=== modified file 'mysql-test/suite/backup/t/backup_errors.test'
--- a/mysql-test/suite/backup/t/backup_errors.test	2009-10-21 12:15:53 +0000
+++ b/mysql-test/suite/backup/t/backup_errors.test	2009-11-06 06:36:22 +0000
@@ -698,11 +698,20 @@ SET SESSION DEBUG='-d';
 --echo #
 
 --echo #
+--echo # Error injection in user privileges checking.
+--echo #
+SET SESSION DEBUG='+d,ER_BACKUP_USER_PRIV_CHECK';
+
+--error ER_BACKUP_USER_PRIV_CHECK
+BACKUP DATABASE db1 TO 'failed1.bak' OVERWRITE;
+SET SESSION DEBUG='-d';
+
+--echo #
 --echo # Error injection in si_object.cc:get_database_stub
 --echo #
 SET SESSION DEBUG='+d,siobj_get_db_stub';
 --error ER_BACKUP_CATALOG_ADD_DB
-BACKUP DATABASE db1 TO 'failed1.bak';
+BACKUP DATABASE db1 TO 'failed1.bak' OVERWRITE;
 SET SESSION DEBUG='-d';
 
 --echo #

=== modified file 'mysql-test/suite/backup_engines/r/backup_interruption.result'
--- a/mysql-test/suite/backup_engines/r/backup_interruption.result	2009-10-09 13:03:56 +0000
+++ b/mysql-test/suite/backup_engines/r/backup_interruption.result	2009-11-06 06:36:22 +0000
@@ -195,9 +195,6 @@ ERROR 70100: Query execution was interru
 SHOW WARNINGS;
 Level	Code	Message
 Error	<error-code>	Query execution was interrupted
-Error	<error-code>	Query execution was interrupted
-Error	<error-code>	Query execution was interrupted
-Error	<error-code>	Query execution was interrupted
 Warning	<error-code>	Operation aborted
 #
 # Examine backup logs.
@@ -413,9 +410,6 @@ ERROR 70100: Query execution was interru
 SHOW WARNINGS;
 Level	Code	Message
 Error	<error-code>	Query execution was interrupted
-Error	<error-code>	Query execution was interrupted
-Error	<error-code>	Query execution was interrupted
-Error	<error-code>	Query execution was interrupted
 Warning	<error-code>	Operation aborted
 #
 # Examine backup logs.

=== modified file 'sql/backup/backup_info.cc'
--- a/sql/backup/backup_info.cc	2009-10-23 15:41:56 +0000
+++ b/sql/backup/backup_info.cc	2009-11-06 06:36:22 +0000
@@ -674,7 +674,18 @@ backup::Image_info::Db* Backup_info::add
   }
 
   // Check to see if the user can see all of the objects in the database.
-  if (obs::check_user_access(m_thd, name))
+  bool has_access= FALSE;
+  bool got_error= obs::check_user_access(m_thd, name, &has_access);
+
+  DBUG_EXECUTE_IF("ER_BACKUP_USER_PRIV_CHECK", got_error= TRUE;);
+
+  if (got_error)
+  {
+    m_log.report_error(ER_BACKUP_USER_PRIV_CHECK, m_thd->security_ctx->user);
+    return NULL;
+  }
+
+  if (!has_access)
   {
     m_log.report_error(ER_BACKUP_ACCESS_OBJS_INCOMPLETE, name->ptr());
     return NULL;
@@ -802,7 +813,18 @@ int Backup_info::add_all_dbs()
   int res= 0;
 
   // Check to see if the user can see all of the databases.
-  if (check_user_access(m_thd, 0))
+  bool has_access= FALSE;
+  bool got_error= obs::check_user_access(m_thd, 0, &has_access);
+
+  DBUG_EXECUTE_IF("ER_BACKUP_USER_PRIV_CHECK", got_error= TRUE;);
+
+  if (got_error)
+  {
+    m_log.report_error(ER_BACKUP_USER_PRIV_CHECK, m_thd->security_ctx->user);
+    return ERROR;
+  }
+
+  if (!has_access)
   {
     m_log.report_error(ER_BACKUP_ACCESS_DBS_INCOMPLETE);
     return ERROR;

=== modified file 'sql/backup/backup_kernel.h'
--- a/sql/backup/backup_kernel.h	2009-10-21 13:32:24 +0000
+++ b/sql/backup/backup_kernel.h	2009-11-06 06:36:22 +0000
@@ -270,7 +270,7 @@ int Backup_restore_ctx::fatal_error(int 
   m_error= error_code;
 
   if (Logger::m_state == RUNNING || Logger::m_state == READY)
-    report_state(BUP_ERRORS);
+    report_state_change(BUP_ERRORS);
 
   return error_code;
 }

=== modified file 'sql/backup/kernel.cc'
--- a/sql/backup/kernel.cc	2009-10-23 15:41:56 +0000
+++ b/sql/backup/kernel.cc	2009-11-06 06:36:22 +0000
@@ -249,7 +249,7 @@ execute_backup_command(THD *thd,
     /* Error condition insertion for ER_BACKUP_BACKUP_PREPARE. */
     DBUG_EXECUTE_IF("ER_BACKUP_BACKUP_PREPARE_2", res= 1;);
 
-    if (res || !info->is_valid())
+    if (context.report_killed() || res || !info->is_valid())
       DBUG_RETURN(send_error(context, ER_BACKUP_BACKUP_PREPARE));
 
     int count= info->db_count();
@@ -347,7 +347,17 @@ execute_backup_command(THD *thd,
 static
 int send_error(Backup_restore_ctx &context, int error_code, ...)
 {
-  if (!context.is_killed() && !context.error_reported())
+  using namespace backup;
+
+  /* Atmost one error should be reported. */
+  if (context.error_reported())
+    return error_code;
+  
+  /* Report error state if not already done. */
+  context.report_state_change(BUP_ERRORS);
+
+  /* If the query has been interrupted, do not print this message. */
+  if (!context.is_killed())
   {
     char buf[MYSQL_ERRMSG_SIZE];
     va_list args;

=== modified file 'sql/backup/logger.h'
--- a/sql/backup/logger.h	2009-10-21 13:32:24 +0000
+++ b/sql/backup/logger.h	2009-11-06 06:36:22 +0000
@@ -68,6 +68,7 @@ public:
    void report_completed(time_t);
    void report_aborted(time_t, bool data_changed);
    void report_state(enum_backup_state);
+   void report_state_change(enum_backup_state);
    void report_vp_time(time_t, bool);
    void report_binlog_info(const st_bstream_binlog_info&);
    void report_master_binlog_info(const st_bstream_binlog_info&);
@@ -263,7 +264,7 @@ void Logger::report_start(time_t when)
   report_error(log_level::INFO, m_type == BACKUP ? ER_BACKUP_BACKUP_START
                                                  : ER_BACKUP_RESTORE_START);
   backup_log->start(when);
-  backup_log->state(BUP_RUNNING);
+  backup_log->set_state(BUP_RUNNING);
 }
 
 
@@ -329,12 +330,14 @@ void Logger::report_aborted(time_t when,
 
 
 /**
-  Report change of the state of operation
+  Report the current state of operation.
 
   For possible states see definition of @c enum_backup_state
 
   @todo Consider reporting state changes in the server error log (as info
   entries).
+
+  @see @c Logger::report_state_change()
 */
 
 inline
@@ -343,11 +346,27 @@ void Logger::report_state(enum_backup_st
   DBUG_ASSERT(m_state == RUNNING || m_state == READY);
   DBUG_ASSERT(backup_log);
 
-  backup_log->state(state);
+  backup_log->set_state(state);
 }
 
 
 /**
+  Report current state of operation only when there is state change.
+  This can be called multiple times without generating duplicate messages.
+
+  For possible states see definition of @c enum_backup_state
+*/
+
+inline
+void Logger::report_state_change(enum_backup_state state)
+{
+  if (!backup_log)
+    return;
+  if (backup_log->get_state() != state)
+    backup_log->set_state(state);
+}
+
+/**
   Report validity point creation time.
 
   @param[in] when   the time of validity point
@@ -443,7 +462,7 @@ int Logger::init(enum_type type, const c
 
   m_type= type;
   backup_log = new Backup_log(m_thd, (enum_backup_operation)type, query);
-  backup_log->state(BUP_STARTING);
+  backup_log->set_state(BUP_STARTING);
   m_state= READY;
   DEBUG_SYNC(m_thd, "after_backup_log_init");
   return 0;

=== modified file 'sql/share/errmsg-utf8.txt'
--- a/sql/share/errmsg-utf8.txt	2009-10-12 09:08:34 +0000
+++ b/sql/share/errmsg-utf8.txt	2009-11-06 06:36:22 +0000
@@ -6562,3 +6562,5 @@ ER_BACKUP_ACCESS_OBJS_INCOMPLETE
   eng "Insufficient privileges. You do not have privileges to backup database '%s'."
 ER_WARN_I_S_SKIPPED_TABLE
   eng "Table '%s'.'%s' was skipped since its definition is being modified by concurrent DDL statement."
+ER_BACKUP_USER_PRIV_CHECK
+  eng "Failed to perform access privileges check for user '%s'."

=== modified file 'sql/share/errmsg.txt'
--- a/sql/share/errmsg.txt	2009-10-26 14:02:26 +0000
+++ b/sql/share/errmsg.txt	2009-11-06 06:36:22 +0000
@@ -6562,3 +6562,5 @@ ER_BACKUP_ACCESS_OBJS_INCOMPLETE
   eng "Insufficient privileges. You do not have privileges to backup database '%s'."
 ER_WARN_I_S_SKIPPED_TABLE
   eng "Table '%s'.'%s' was skipped since its definition is being modified by concurrent DDL statement."
+ER_BACKUP_USER_PRIV_CHECK
+  eng "Failed to perform access privileges check for user '%s'."

=== modified file 'sql/si_logs.cc'
--- a/sql/si_logs.cc	2009-05-21 13:17:37 +0000
+++ b/sql/si_logs.cc	2009-11-06 06:36:22 +0000
@@ -162,13 +162,24 @@ bool Backup_log::write_master_binlog_inf
   @todo Consider reporting state changes in the server error log (as info
   entries).
  */
-void Backup_log::state(enum_backup_state state)
+void Backup_log::set_state(enum_backup_state state)
 {
   m_op_hist.state= state;
   logger.backup_progress_log_write(m_thd, m_op_hist.backup_id, "backup kernel", 0, 
                             0, 0, 0, 0, get_state_string(state));
 }
 
+/** 
+  Get current state of operation.
+ 
+  For possible states see definition of @c enum_backup_state 
+ */
+enum_backup_state Backup_log::get_state()
+{
+  return m_op_hist.state;
+}
+
+
 /**
   Report validity point creation time.
 

=== modified file 'sql/si_logs.h'
--- a/sql/si_logs.h	2009-05-21 13:17:37 +0000
+++ b/sql/si_logs.h	2009-11-06 06:36:22 +0000
@@ -130,7 +130,8 @@ public:
     the backup_history log.
   */
   ulonglong get_backup_id() { return m_op_hist.backup_id; }
-  void state(enum_backup_state);
+  void set_state(enum_backup_state);
+  enum_backup_state get_state();
   void error_num(int code) { m_op_hist.error_num= code; }
   void binlog_start_pos(unsigned long int pos) 
   { 

=== modified file 'sql/si_objects.cc'
--- a/sql/si_objects.cc	2009-11-04 14:18:21 +0000
+++ b/sql/si_objects.cc	2009-11-06 06:36:22 +0000
@@ -236,7 +236,8 @@ run_service_interface_sql(THD *thd, Ed_c
   
   bool rc= ed_connection->execute_direct(*query);
 
-  if (get_warnings) 
+  /* Skip append warnings if query was killed to avoid duplicates. */
+  if (get_warnings && !thd->killed)
   {
     /* Push warnings on the THD error stack. */
     thd->warning_info->append_warnings(thd, ed_connection->get_warn_list());
@@ -1310,11 +1311,6 @@ Iterator *create_row_set_iterator(THD *t
   if (run_service_interface_sql(thd, &ed_connection, query, TRUE))
     /* Query failed with an error */
     return NULL;
-  else if(ed_connection.get_warn_count())
-    /* Push warnings to BACKUP's error stack. Calling method will
-       decide if the warnings are a problem later when serializing the
-       object. */
-    thd->warning_info->append_warnings(thd, ed_connection.get_warn_list());
 
   DBUG_ASSERT(ed_connection.get_field_count());
 
@@ -2561,9 +2557,9 @@ Obj_iterator *get_databases(THD *thd)
 
   @param[in] THD     Current thread
   
-  @return uint Number of databases.
+  @return int Number of databases. Return value of -1 indicates failure.
 */
-uint get_num_dbs(THD *thd)
+int get_num_dbs(THD *thd)
 {
   Ed_connection ed_connection(thd);
   Ed_result_set *ed_result_set;
@@ -2578,10 +2574,7 @@ uint get_num_dbs(THD *thd)
   if (run_service_interface_sql(thd, &ed_connection,
 				s_stream.lex_string(), TRUE))
     /* Query failed with an error */
-    return NULL;
-  else if(ed_connection.get_warn_count())
-    /* Push warnings to BACKUP's error stack. */
-    thd->warning_info->append_warnings(thd, ed_connection.get_warn_list());
+    return -1;
 
   DBUG_ASSERT(ed_connection.get_field_count());
 
@@ -2607,9 +2600,9 @@ uint get_num_dbs(THD *thd)
   @param[in] THD     Current thread
   @param[in] String  Database name.
 
-  @return uint Number of objects in the database.
+  @return int Number of objects in the database. Value of -1 means failure.
 */
-uint get_num_objects(THD *thd, const String *db_name)
+int get_num_objects(THD *thd, const String *db_name)
 {
   Ed_connection ed_connection(thd);
   Ed_result_set *ed_result_set;
@@ -2635,10 +2628,7 @@ uint get_num_objects(THD *thd, const Str
   if (run_service_interface_sql(thd, &ed_connection, 
 				s_stream.lex_string(), TRUE))
     /* Query failed with an error */
-    return NULL;
-  else if(ed_connection.get_warn_count())
-    /* Push warnings to BACKUP's error stack. */
-    thd->warning_info->append_warnings(thd, ed_connection.get_warn_list());
+    return -1;
 
   DBUG_ASSERT(ed_connection.get_field_count());
 
@@ -2650,10 +2640,13 @@ uint get_num_objects(THD *thd, const Str
 }
 
 ///////////////////////////////////////////////////////////////////////////
-bool check_user_access(THD *thd, const String *db_name)
+bool check_user_access(THD *thd, const String *db_name, bool *has_access)
 {
-  uint elev_count;
-  uint user_count;
+  int elev_count;
+  int user_count;
+
+  DBUG_ASSERT(has_access);
+  *has_access= false;                           /* Reset access status. */
 
   /*
     Get value from user context without elevation.
@@ -2663,6 +2656,9 @@ bool check_user_access(THD *thd, const S
   else 
     user_count= get_num_objects(thd, db_name);
 
+  if (user_count < 0)
+    return TRUE;                                /* Return error status */
+
   /* 
     Peform global elevation with SELECT to get value of
     someone with global SELECT (e.g. root).
@@ -2682,10 +2678,13 @@ bool check_user_access(THD *thd, const S
   thd->security_ctx->master_access= saved_master_access; 
   thd->security_ctx->db_access= saved_db_access; 
 
-  /*
-    Compare results and return.
-  */
-  return !(user_count == elev_count);
+  if (elev_count < 0)
+    return TRUE;                                /* Return error status */
+
+  /* User has proper access if both counts are same. */
+  *has_access= (user_count == elev_count);
+
+  return FALSE;
 }
 
 ///////////////////////////////////////////////////////////////////////////

=== modified file 'sql/si_objects.h'
--- a/sql/si_objects.h	2009-11-04 14:18:21 +0000
+++ b/sql/si_objects.h	2009-11-06 06:36:22 +0000
@@ -185,18 +185,17 @@ Obj_iterator *get_databases(THD *thd);
   2) If db_name != 0, the method shall check the user's visibility for all
      objects in the database specified. 
  
-  @param[in] THD     The current thread.
-  @param[in] String  The database name to check or 0
-
-  @returns Information whether user can see all specified objects
-    @retval TRUE
-      if db_name != 0 : user does not have visibility to the database specified
-      if db_name == 0 : user does not have visibility to all databases
-    @retval FALSE
-      if db_name != 0 : user does have visibility to the database specified
-      if db_name == 0 : user does have visibility to all databases
+  @param[in]  THD     The current thread.
+  @param[in]  String  The database name to check or 0
+  @param[out] bool    Set to true/false as described below:
+                      TRUE  : User has the required access permission.
+                      FALSE : User does not have the required access permission.
+
+  @return boolean value which indicates if an error has occured.
+    @retval TRUE error has occured.
+    @retval FALSE function completed without error.
 */
-bool check_user_access(THD *thd, const String *db_name);
+bool check_user_access(THD *thd, const String *db_name, bool *has_access);
 
 /**
   Create an iterator over all base tables in a particular database.

Thread
bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888) Bug#47994Thava Alagu6 Nov
  • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Ingo Strüwing6 Nov
    • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Rafal Somla6 Nov
    • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Thava Alagu8 Nov
      • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Ingo Strüwing9 Nov
  • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Rafal Somla6 Nov
    • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Thava Alagu8 Nov
      • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Rafal Somla9 Nov
        • Re: bzr commit into mysql-6.0-backup branch (thavamuni.alagu:2888)Bug#47994Ingo Strüwing9 Nov