Review of changesets since last pull.

(Mostly code cleanups)


include/my_sys.h:
  Removed not used define
myisam/mi_check.c:
  Indentation change
mysql-test/t/rpl_insert_id.test:
  Add test if server supports innodb
mysys/mf_format.c:
  Remove QUOTE handling from fn_format()
  (fn_format() should not have anything to do with quoting things)
sql/log_event.cc:
  Removed some unnecessary casts (by changing functions to use const char *)
  Cleaned up some error messages to make them shorter and (hopefully) more readable.
  Fixed wrong format strings
  Restored 'thd->options' on error.
sql/mini_client.cc:
  Changed order of include files to remove compilation warning
sql/mysqld.cc:
  Add proper quoting of service arguments (don't use fn_format)
sql/slave.cc:
  Simple optimization and cleanup.
  Changed rewrite_db() and print_slave_db_safe() to use const char* to avoid casting of arguments when calling functions.
  Cleanup of some error messages.
sql/slave.h:
  Changed protypes to use const char *
sql/sql_acl.cc:
  Simple optimization
vio/viosslfactories.c:
  Changed order of include files to remove compilation warning

include/my_sys.h

@@ -100,7 +100,6 @@ extern int NEAR my_errno;		/* Last error in mysys */
 #define MY_RETURN_REAL_PATH	32	/* return full path for file */
 #define MY_SAFE_PATH		64	/* Return NULL if too long path */
 #define MY_RELATIVE_PATH	128	/* name is relative to 'dir' */
-#define MY_QUOTE_SPACES        256     /* quote when the path has spaces */
 
 	/* My seek flags */
 #define MY_SEEK_SET	0

myisam/mi_check.c

@@ -3563,11 +3563,13 @@ int update_state_info(MI_CHECK *param, MI_INFO *info,uint update)
     share->state.rec_per_key_rows=info->state->records;
     share->state.changed&= ~STATE_NOT_ANALYZED;
     if (info->state->records)
+    {
       for (i=0; i<key_parts; i++)
       {
         if (!(share->state.rec_per_key_part[i]=param->rec_per_key_part[i]))
           share->state.changed|= STATE_NOT_ANALYZED;
       }
+    }
   }
   if (update & (UPDATE_STAT | UPDATE_SORT | UPDATE_TIME | UPDATE_AUTO_INC))
   {

mysql-test/t/rpl_insert_id.test

@@ -4,6 +4,7 @@
 # We also check how the foreign_key_check variable is replicated
 
 source include/master-slave.inc;
+source include/have_innodb.inc
 connection master;
 drop table if exists t1;
 create table t1(a int auto_increment, key(a));

mysys/mf_format.c

@@ -111,15 +111,7 @@ my_string fn_format(my_string to, const char *name, const char *dir,
     strmov(buff,to);
     (void) my_readlink(to, buff, MYF(0));
   }
-  if ( flag & MY_QUOTE_SPACES)
-    if ( strchr(to, ' '))
-    {
-      char tmp_buff[FN_REFLEN];
-      tmp_buff[0]='"';
-      strxmov(tmp_buff+1,to,"\"",NullS);
-      strmov(to,tmp_buff);
-    }  
-  DBUG_RETURN (to);
+  DBUG_RETURN(to);
 } /* fn_format */
 
 

sql/log_event.cc

@@ -593,8 +593,9 @@ err:
   UNLOCK_MUTEX;
   if (error)
   {
-    sql_print_error("Error in Log_event::read_log_event(): '%s', \
-data_len=%d,event_type=%d",error,data_len,head[EVENT_TYPE_OFFSET]);
+    sql_print_error("\
+Error in Log_event::read_log_event(): '%s', data_len: %d, event_type: %d",
+		    error,data_len,head[EVENT_TYPE_OFFSET]);
     my_free(buf, MYF(MY_ALLOW_ZERO_PTR));
     /*
       The SQL slave thread will check if file->error<0 to know
@@ -1433,8 +1434,8 @@ void Slave_log_event::print(FILE* file, bool short_form, char* last_db)
     return;
   print_header(file);
   fputc('\n', file);
-  fprintf(file, "Slave: master_host: '%s'  master_port: %d  \
-master_log: '%s'  master_pos: %s\n",
+  fprintf(file, "\
+Slave: master_host: '%s'  master_port: %d  master_log: '%s'  master_pos: %s\n",
 	  master_host, master_port, master_log, llstr(master_pos, llbuff));
 }
 
@@ -1755,7 +1756,7 @@ int Query_log_event::exec_event(struct st_relay_log_info* rli)
 {
   int expected_error, actual_error= 0;
   init_sql_alloc(&thd->mem_root, 8192,0);
-  thd->db = rewrite_db((char*)db);
+  thd->db= (char*) rewrite_db(db);
 
   /*
     InnoDB internally stores the master log position it has processed so far;
@@ -1811,15 +1812,16 @@ int Query_log_event::exec_event(struct st_relay_log_info* rli)
 	  !ignored_error_code(expected_error))
       {
 	slave_print_error(rli, 0,
-                          "Query '%s' did not get the same error as the query \
-got on master - got on master: '%s' (%d), got on slave: '%s' (%d) \
-(default database was '%s')",
+                          "\
+Query '%s' caused different errors on master and slave. \
+Error on master: '%s' (%d), Error on slave: '%s' (%d). \
+Default database: '%s'",
                           query,
                           ER_SAFE(expected_error),
                           expected_error,
                           actual_error ? thd->net.last_error: "no error",
                           actual_error,
-                          print_slave_db_safe((char*)db));
+                          print_slave_db_safe(db));
 	thd->query_error= 1;
       }
       /*
@@ -1837,11 +1839,12 @@ got on master - got on master: '%s' (%d), got on slave: '%s' (%d) \
       */
       else if (thd->query_error || thd->fatal_error)
       {
-        slave_print_error(rli,actual_error, "Error '%s' on query '%s' \
-(default database was '%s')",
-                          actual_error ? thd->net.last_error :
-                          "unexpected success or fatal error", query,
-                          print_slave_db_safe((char*)db)); 
+        slave_print_error(rli,actual_error,
+			  "Error '%s' on query '%s'. Default database: '%s'",
+                          (actual_error ? thd->net.last_error :
+			   "unexpected success or fatal error"),
+			  query,
+                          print_slave_db_safe(db));
         thd->query_error= 1;
       }
     } 
@@ -1896,7 +1899,7 @@ int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
 			       bool use_rli_only_for_errors)
 {
   init_sql_alloc(&thd->mem_root, 8192,0);
-  thd->db = rewrite_db((char*)db);
+  thd->db= (char*) rewrite_db(db);
   DBUG_ASSERT(thd->query == 0);
   thd->query = 0;				// Should not be needed
   thd->query_error = 0;
@@ -1978,17 +1981,15 @@ int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
       if (mysql_load(thd, &ex, &tables, field_list, handle_dup, net != 0,
 		     TL_WRITE))
 	thd->query_error = 1;
+      /* log_pos is the position of the LOAD event in the master log */
       if (thd->cuted_fields)
-	/* 
-	   log_pos is the position of the LOAD
-	   event in the master log
-	*/
-	sql_print_error("Slave: load data infile at position %s in log \
-'%s' produced %d warning(s) (loaded table was '%s', database was '%s')",
+	sql_print_error("\
+Slave: load data infile on table '%s' at log position %s in log \
+'%s' produced %ld warning(s). Default database: '%s'",
+                        (char*) table_name,
                         llstr(log_pos,llbuff), RPL_LOG_NAME, 
-			thd->cuted_fields,
-                        (char*)table_name,
-                        print_slave_db_safe((char*)db));
+			(ulong) thd->cuted_fields,
+                        print_slave_db_safe(db));
       if (net)
         net->pkt_nr= thd->net.pkt_nr;
     }
@@ -2019,10 +2020,9 @@ int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
       sql_errno=ER_UNKNOWN_ERROR;
       err=ER(sql_errno);       
     }
-    slave_print_error(rli,sql_errno,
-		      "Error '%s' running load data infile \
-(loaded table was '%s', database was '%s')",
-		      err, (char*)table_name, print_slave_db_safe((char*)db));
+    slave_print_error(rli,sql_errno,"\
+Error '%s' running lOAD DATA INFILE on table '%s'. Default database: '%s'",
+		      err, (char*)table_name, print_slave_db_safe(db));
     free_root(&thd->mem_root,0);
     return 1;
   }
@@ -2030,10 +2030,9 @@ int Load_log_event::exec_event(NET* net, struct st_relay_log_info* rli,
 	    
   if (thd->fatal_error)
   {
-    slave_print_error(rli,ER_UNKNOWN_ERROR,
-"Fatal error running \
-LOAD DATA INFILE (loaded table was '%s', database was '%s')",
-                    (char*)table_name, print_slave_db_safe((char*)db));
+    slave_print_error(rli,ER_UNKNOWN_ERROR, "\
+Fatal error running LOAD DATA INFILE on table '%s'. Default database: '%s'",
+		      (char*)table_name, print_slave_db_safe(db));
     return 1;
   }
 
@@ -2339,13 +2338,15 @@ int Execute_load_log_event::exec_event(struct st_relay_log_info* rli)
       What we want instead is add the filename to the current error message.
     */
     char *tmp= my_strdup(rli->last_slave_error,MYF(MY_WME));
-    if (!tmp)
-      goto err;
-    slave_print_error(rli,rli->last_slave_errno, /* ok to re-use the error code */
-                      "%s. Failed executing load from '%s'", 
-                      tmp, fname);
-    my_free(tmp,MYF(0));
-    thd->options = save_options;
+    if (tmp)
+    {
+      slave_print_error(rli,
+			rli->last_slave_errno, /* ok to re-use error code */
+			"%s. Failed executing load from '%s'", 
+			tmp, fname);
+      my_free(tmp,MYF(0));
+    }
+    thd->options= save_options;
     goto err;
   }
   thd->options = save_options;

sql/mini_client.cc

@@ -30,8 +30,8 @@
 #include <thr_alarm.h>
 #include <mysql_embed.h>
 #include <mysql_com.h>
-#include <violite.h>
 #include <my_sys.h>
+#include <violite.h>
 #include <mysys_err.h>
 #include <m_string.h>
 #include <m_ctype.h>

sql/mysqld.cc

@@ -2506,6 +2506,19 @@ int mysql_service(void *p)
   return 0;
 }
 
+
+/* Quote string if it contains space, else copy */
+
+static char *add_quoted_string(char *to, const char *from, char *to_end)
+{
+  uint length= (uint) (to_end-to);
+
+  if (!strchr(from, ' '))
+    return strnmov(to, from, length);
+  return strxnmov(to, length, "\"", from, "\"", NullS);
+}
+
+
 /*
   Handle basic handling of services, like installation and removal
 
@@ -2515,25 +2528,41 @@ int mysql_service(void *p)
     servicename		Internal name of service
     displayname		Display name of service (in taskbar ?)
     file_path		Path to this program
+    startup_option	Startup option to mysqld
 
   RETURN VALUES
     0		option handled
     1		Could not handle option
  */
 
-bool default_service_handling(char **argv,
-			      const char *servicename,
-			      const char *displayname,
-			      const char *file_path)
+static bool
+default_service_handling(char **argv,
+			 const char *servicename,
+			 const char *displayname,
+			 const char *file_path,
+			 const char *extra_opt)
 {
+  char path_and_service[FN_REFLEN+FN_REFLEN+32], *pos, *end;
+  end= path_and_service + sizeof(path_and_service)-1;
+
+  /* We have to quote filename if it contains spaces */
+  pos= add_quoted_string(path_and_service, file_path, end);
+  if (*extra_opt)
+  {
+    /* Add (possible quoted) option after file_path */
+    *pos++= ' ';
+    pos= add_quoted_string(pos, extra_opt, end);
+  }
+  *pos= 0;					// Ensure end null
+
   if (Service.got_service_option(argv, "install"))
   {
-    Service.Install(1, servicename, displayname, file_path);
+    Service.Install(1, servicename, displayname, path_and_service);
     return 0;
   }
   if (Service.got_service_option(argv, "install-manual"))
   {
-    Service.Install(0, servicename, displayname, file_path);
+    Service.Install(0, servicename, displayname, path_and_service);
     return 0;
   }
   if (Service.got_service_option(argv, "remove"))
@@ -2560,12 +2589,11 @@ int main(int argc, char **argv)
     char file_path[FN_REFLEN];
     my_path(file_path, argv[0], "");		      /* Find name in path */
     fn_format(file_path,argv[0],file_path,"",MY_REPLACE_DIR+
-              MY_UNPACK_FILENAME+MY_RESOLVE_SYMLINKS+MY_QUOTE_SPACES);
-
+              MY_UNPACK_FILENAME | MY_RESOLVE_SYMLINKS);
     if (argc == 2)
     {	
-      if (!default_service_handling(argv,MYSQL_SERVICENAME, MYSQL_SERVICENAME,
-				   file_path))
+      if (!default_service_handling(argv, MYSQL_SERVICENAME, MYSQL_SERVICENAME,
+				   file_path, ""))
 	return 0;
       if (Service.IsService(argv[1]))
       {
@@ -2578,12 +2606,8 @@ int main(int argc, char **argv)
     }
     else if (argc == 3) /* install or remove any optional service */
     {
-      /* Add service name after filename */
-      uint length=strlen(file_path);
-      *strxnmov(file_path + length, sizeof(file_path)-length-2, " ",
-		argv[2], NullS)= '\0';
-
-      if (!default_service_handling(argv, argv[2], argv[2], file_path))
+      if (!default_service_handling(argv, argv[2], argv[2], file_path,
+				    argv[2]))
 	return 0;
       if (Service.IsService(argv[2]))
       {
@@ -2605,12 +2629,8 @@ int main(int argc, char **argv)
 	Install an optional service with optional config file
 	mysqld --install-manual mysqldopt --defaults-file=c:\miguel\my.ini
       */
-      uint length=strlen(file_path);
-      char tmp_path[FN_REFLEN];
-      fn_format(tmp_path,argv[3],tmp_path,"",MY_QUOTE_SPACES);
-      *strxnmov(file_path + length, sizeof(file_path)-length-2, " ",
-                tmp_path, " ", argv[2], NullS)= '\0';
-      if (!default_service_handling(argv, argv[2], argv[2], file_path))
+      if (!default_service_handling(argv, argv[2], argv[2], file_path,
+				    argv[3]))
 	return 0;
     }
     else if (argc == 1 && Service.IsService(MYSQL_SERVICENAME))

sql/slave.cc

@@ -828,6 +828,7 @@ static bool sql_slave_killed(THD* thd, RELAY_LOG_INFO* rli)
   return rli->abort_slave || abort_loop || thd->killed;
 }
 
+
 /*
   Writes an error message to rli->last_slave_error and rli->last_slave_errno
   (which will be displayed by SHOW SLAVE STATUS), and prints it to stderr.
@@ -842,7 +843,7 @@ static bool sql_slave_killed(THD* thd, RELAY_LOG_INFO* rli)
 
   RETURN VALUES
     void
- */
+*/
 
 void slave_print_error(RELAY_LOG_INFO* rli, int err_code, const char* msg, ...)
 {
@@ -853,11 +854,11 @@ void slave_print_error(RELAY_LOG_INFO* rli, int err_code, const char* msg, ...)
   rli->last_slave_errno = err_code;
   /* If the error string ends with '.', do not add a ',' it would be ugly */
   if (rli->last_slave_error[0] && 
-      (rli->last_slave_error[strlen(rli->last_slave_error)-1] == '.'))
-    sql_print_error("Slave: %s Error_code=%d", rli->last_slave_error,
+      (*(strend(rli->last_slave_error)-1) == '.'))
+    sql_print_error("Slave: %s Error_code: %d", rli->last_slave_error,
                     err_code);
   else
-    sql_print_error("Slave: %s, error_code=%d", rli->last_slave_error,
+    sql_print_error("Slave: %s, Error_code: %d", rli->last_slave_error,
                     err_code);
 
 }
@@ -872,7 +873,7 @@ void skip_load_data_infile(NET* net)
 }
 
 
-char* rewrite_db(char* db)
+const char *rewrite_db(const char* db)
 {
   if (replicate_rewrite_db.is_empty() || !db)
     return db;
@@ -889,13 +890,14 @@ char* rewrite_db(char* db)
 
 /*
   From other comments and tests in code, it looks like
-  sometimes Query_log_event and Load_log_event can have db==0
+  sometimes Query_log_event and Load_log_event can have db == 0
   (see rewrite_db() above for example)
   (cases where this happens are unclear; it may be when the master is 3.23).
 */
-char* print_slave_db_safe(char* db)
+
+const char *print_slave_db_safe(const char* db)
 {
-  return (db ? rewrite_db(db) : (char*) "");
+  return (db ? rewrite_db(db) : "");
 }
 
 /*
@@ -1303,8 +1305,8 @@ file '%s', errno %d)", fname, my_errno);
     if (init_io_cache(&rli->info_file, info_fd, IO_SIZE*2, READ_CACHE, 0L,0,
 		      MYF(MY_WME))) 
     {
-      sql_print_error("Failed to create a cache on relay log info file (\
-file '%s')", fname);
+      sql_print_error("Failed to create a cache on relay log info file '%s'",
+		      fname);
       msg= current_thd->net.last_error;
       goto err;
     }
@@ -1313,8 +1315,7 @@ file '%s')", fname);
     if (init_relay_log_pos(rli,NullS,BIN_LOG_HEADER_SIZE,0 /* no data lock */,
 			   &msg))
     {
-      sql_print_error("Failed to open the relay log (relay_log_name='FIRST', \
-relay_log_pos=4)");
+      sql_print_error("Failed to open the relay log 'FIRST' (relay_log_pos 4");
       goto err;
     }
     rli->master_log_name[0]= 0;
@@ -1330,15 +1331,16 @@ relay_log_pos=4)");
       int error=0;
       if ((info_fd = my_open(fname, O_RDWR|O_BINARY, MYF(MY_WME))) < 0)
       {
-        sql_print_error("Failed to open the existing relay log info file (\
-file '%s', errno %d)", fname, my_errno);
+        sql_print_error("\
+Failed to open the existing relay log info file '%s' (errno %d)",
+			fname, my_errno);
         error= 1;
       }
       else if (init_io_cache(&rli->info_file, info_fd,
                              IO_SIZE*2, READ_CACHE, 0L, 0, MYF(MY_WME)))
       {
-        sql_print_error("Failed to create a cache on relay log info file (\
-file '%s')", fname);
+        sql_print_error("Failed to create a cache on relay log info file '%s'",
+			fname);
         error= 1;
       }
       if (error)
@@ -1377,8 +1379,8 @@ file '%s')", fname);
 			   &msg))
     {
       char llbuf[22];
-      sql_print_error("Failed to open the relay log (relay_log_name='%s', \
-relay_log_pos=%s)", rli->relay_log_name, llstr(rli->relay_log_pos, llbuf));
+      sql_print_error("Failed to open the relay log '%s' (relay_log_pos %s)",
+		      rli->relay_log_name, llstr(rli->relay_log_pos, llbuf));
       goto err;
     }
   }

sql/slave.h

@@ -382,8 +382,8 @@ int add_table_rule(HASH* h, const char* table_spec);
 int add_wild_table_rule(DYNAMIC_ARRAY* a, const char* table_spec);
 void init_table_rule_hash(HASH* h, bool* h_inited);
 void init_table_rule_array(DYNAMIC_ARRAY* a, bool* a_inited);
-char* rewrite_db(char* db);
-char* print_slave_db_safe(char* db);
+const char *rewrite_db(const char* db);
+const char *print_slave_db_safe(const char* db);
 int check_expected_error(THD* thd, RELAY_LOG_INFO* rli, int error_code);
 void skip_load_data_infile(NET* net);
 void slave_print_error(RELAY_LOG_INFO* rli, int err_code, const char* msg, ...);

sql/sql_acl.cc

@@ -1242,13 +1242,12 @@ static bool update_user_table(THD *thd, const char *host, const char *user,
   {
     /* 
        The tables must be marked "updating" so that tables_ok() takes them into
-       account in tests.
+       account in tests.  It's ok to leave 'updating' set after tables_ok.
     */
-    tables.updating=1;
+    tables.updating= 1;
     /* Thanks to bzero, tables.next==0 */
     if (!tables_ok(0, &tables))
       DBUG_RETURN(0);
-    tables.updating=0;
   }
 #endif
 
@@ -2138,10 +2137,9 @@ int mysql_table_grant(THD *thd, TABLE_LIST *table_list,
        The tables must be marked "updating" so that tables_ok() takes them into
        account in tests.
     */
-    tables[0].updating=tables[1].updating=tables[2].updating=1;
+    tables[0].updating= tables[1].updating= tables[2].updating= 1;
     if (!tables_ok(0, tables))
       DBUG_RETURN(0);
-    tables[0].updating=tables[1].updating=tables[2].updating=0;
   }
 #endif
 
@@ -2319,10 +2317,9 @@ int mysql_grant (THD *thd, const char *db, List <LEX_USER> &list,
        The tables must be marked "updating" so that tables_ok() takes them into
        account in tests.
     */
-    tables[0].updating=tables[1].updating=1;
+    tables[0].updating= tables[1].updating= 1;
     if (!tables_ok(0, tables))
       DBUG_RETURN(0);
-    tables[0].updating=tables[1].updating=0;
   }
 #endif
 

vio/viosslfactories.c

@@ -18,8 +18,8 @@
 
 #ifdef HAVE_OPENSSL
 
-#include <my_sys.h>
 #include <mysql_com.h>
+#include <my_sys.h>
 #include <violite.h>