Cleanups during review of code

Fixed newly introduced bug in rollup


client/mysqldump.c:
  Safer buffer allocation
  Removed wrong assert
mysql-test/r/olap.result:
  more tests
mysql-test/t/olap.test:
  more tests
sql/handler.cc:
  Simple cleanup
  Fixed wrong check for next digit (wrong debug output)
sql/item.cc:
  Replace shrink_to_length() with mark_as_const() as the former allowed one to do changes to the string
sql/item_sum.cc:
  Change reference to pointer
  Trivial optimzation of testing 'allways_null'
sql/mysqld.cc:
  Proper indentation of comment
sql/sql_select.cc:
  Fixed newly introduced bug in rollup
sql/sql_string.h:
  Remove not needed 'shrink_to_length()'
  Added 'mark_as_const()' to be used when one want to ensure that a string is not changed

client/mysqldump.c

@@ -1093,7 +1093,7 @@ static void print_xml_row(FILE *xml_file, const char *row_name,
     number of fields in table, 0 if error
 */
 
-static uint getTableStructure(char *table, char* db)
+static uint getTableStructure(char *table, char *db)
 {
   MYSQL_RES  *tableRes;
   MYSQL_ROW  row;
@@ -1396,10 +1396,9 @@ static uint getTableStructure(char *table, char* db)
       /* Get MySQL specific create options */
       if (create_options)
       {
-	char show_name_buff[FN_REFLEN];
+	char show_name_buff[NAME_LEN*2+2+24];
 
 	/* Check memory for quote_for_like() */
-	DBUG_ASSERT(2*sizeof(table) < sizeof(show_name_buff));
         my_snprintf(buff, sizeof(buff), "show table status like %s",
 		    quote_for_like(table, show_name_buff));
 

mysql-test/r/olap.result

@@ -378,6 +378,51 @@ a	sum(b)
 2	6
 4	4
 NULL	14
+SELECT b, a, sum(b) FROM t1 GROUP BY a,b WITH ROLLUP;
+b	a	sum(b)
+4	1	4
+NULL	1	4
+1	2	2
+2	2	4
+NULL	2	6
+1	4	4
+NULL	4	4
+NULL	NULL	14
+SELECT DISTINCT b,a, sum(b) FROM t1 GROUP BY a,b WITH ROLLUP;
+b	a	sum(b)
+4	1	4
+NULL	1	4
+1	2	2
+2	2	4
+NULL	2	6
+1	4	4
+NULL	4	4
+NULL	NULL	14
+ALTER TABLE t1 ADD COLUMN c INT;
+SELECT a,b,sum(c) FROM t1 GROUP BY a,b,c WITH ROLLUP;
+a	b	sum(c)
+1	4	NULL
+1	4	NULL
+1	NULL	NULL
+2	1	NULL
+2	1	NULL
+2	2	NULL
+2	2	NULL
+2	NULL	NULL
+4	1	NULL
+4	1	NULL
+4	NULL	NULL
+NULL	NULL	NULL
+SELECT distinct a,b,sum(c) FROM t1 GROUP BY a,b,c WITH ROLLUP;
+a	b	sum(c)
+1	4	NULL
+1	NULL	NULL
+2	1	NULL
+2	2	NULL
+2	NULL	NULL
+4	1	NULL
+4	NULL	NULL
+NULL	NULL	NULL
 DROP TABLE t1;
 CREATE TABLE t1 (a int, b int);
 INSERT INTO t1 VALUES

mysql-test/t/olap.test

@@ -153,6 +153,13 @@ SELECT DISTINCT SUM(b), COUNT(DISTINCT b), COUNT(*) FROM t1
 SELECT a, sum(b) FROM t1 GROUP BY a,b WITH ROLLUP;
 SELECT DISTINCT a, sum(b) FROM t1 GROUP BY a,b WITH ROLLUP;
 
+SELECT b, a, sum(b) FROM t1 GROUP BY a,b WITH ROLLUP;
+SELECT DISTINCT b,a, sum(b) FROM t1 GROUP BY a,b WITH ROLLUP;
+
+ALTER TABLE t1 ADD COLUMN c INT;
+SELECT a,b,sum(c) FROM t1 GROUP BY a,b,c WITH ROLLUP;
+SELECT distinct a,b,sum(c) FROM t1 GROUP BY a,b,c WITH ROLLUP;
+
 DROP TABLE t1;
 
 #

sql/handler.cc

@@ -762,14 +762,13 @@ static char* xid_to_str(char *buf, XID *xid)
   for (i=0; i < xid->gtrid_length+xid->bqual_length; i++)
   {
     uchar c=(uchar)xid->data[i];
-    bool is_next_dig;
+    /* is_next_dig is set if next character is a number */
+    bool is_next_dig= FALSE;
     if (i < XIDDATASIZE)
     {
-      char ch=xid->data[i+1];
-      is_next_dig=(c >= '0' && c <='9');
+      char ch= xid->data[i+1];
+      is_next_dig= (ch >= '0' && ch <='9');
     }
-    else
-      is_next_dig=FALSE;
     if (i == xid->gtrid_length)
     {
       *s++='\'';
@@ -782,6 +781,11 @@ static char* xid_to_str(char *buf, XID *xid)
     if (c < 32 || c > 126)
     {
       *s++='\\';
+      /*
+        If next character is a number, write current character with
+        3 octal numbers to ensure that the next number is not seen
+        as part of the octal number
+      */
       if (c > 077 || is_next_dig)
         *s++=_dig_vec_lower[c >> 6];
       if (c > 007 || is_next_dig)

sql/item.cc

@@ -586,18 +586,8 @@ Item *Item_string::safe_charset_converter(CHARSET_INFO *tocs)
     return NULL;
   }
   conv->str_value.copy();
-  /* 
-    The above line executes str_value.realloc() internally,
-    which alligns Alloced_length using ALLIGN_SIZE.
-    In the case of Item_string::str_value we don't want
-    Alloced_length to be longer than str_length.
-    Otherwise, some functions like Item_func_concat::val_str()
-    try to reuse str_value as a buffer for concatenation result
-    for optimization purposes, so our string constant become
-    corrupted. See bug#8785 for more details.
-    Let's shrink Alloced_length to str_length to avoid this problem.
-  */
-  conv->str_value.shrink_to_length();
+  /* Ensure that no one is going to change the result string */
+  conv->str_value.mark_as_const();
   return conv;
 }
 

sql/item_sum.cc

@@ -2671,11 +2671,11 @@ int dump_leaf_key(byte* key, uint32 count __attribute__((unused)),
   TABLE *table= item->table;
   char *record= (char*) table->record[0] + table->s->null_bytes;
   String tmp(table->record[1], table->s->reclength, default_charset_info), tmp2;
-  String &result= item->result;
+  String *result= &item->result;
   Item **arg= item->args, **arg_end= item->args + item->arg_count_field;
 
-  if (result.length())
-    result.append(*item->separator);
+  if (result->length())
+    result->append(*item->separator);
 
   tmp.length(0);
 
@@ -2702,14 +2702,14 @@ int dump_leaf_key(byte* key, uint32 count __attribute__((unused)),
     else
       res= (*arg)->val_str(&tmp);
     if (res)
-      result.append(*res);
+      result->append(*res);
   }
 
   /* stop if length of result more than max_length */
-  if (result.length() > item->max_length)
+  if (result->length() > item->max_length)
   {
     item->count_cut_values++;
-    result.length(item->max_length);
+    result->length(item->max_length);
     item->warning_for_row= TRUE;
     return 1;
   }
@@ -2910,8 +2910,6 @@ Item_func_group_concat::fix_fields(THD *thd, TABLE_LIST *tables, Item **ref)
                MYF(0));
     return TRUE;
   }
-  if (!args)                      /* allocation in constructor may fail */
-    return TRUE;
 
   thd->allow_sum_func= 0;
   maybe_null= 0;
@@ -2972,12 +2970,10 @@ bool Item_func_group_concat::setup(THD *thd)
       if (item->null_value)
       {
         always_null= 1;
-        break;
+        DBUG_RETURN(FALSE);
       }
     }
   }
-  if (always_null)
-    DBUG_RETURN(FALSE);
 
   List<Item> all_fields(list);
   /*

sql/mysqld.cc

@@ -3310,9 +3310,10 @@ default_service_handling(char **argv,
 int main(int argc, char **argv)
 {
 
-  /* When several instances are running on the same machine, we
-     need to have an  unique  named  hEventShudown  through the
-     application PID e.g.: MySQLShutdown1890; MySQLShutdown2342
+  /*
+    When several instances are running on the same machine, we
+    need to have an  unique  named  hEventShudown  through the
+    application PID e.g.: MySQLShutdown1890; MySQLShutdown2342
   */
   int10_to_str((int) GetCurrentProcessId(),strmov(shutdown_event_name,
                                                   "MySQLShutdown"), 10);

sql/sql_select.cc

@@ -10145,20 +10145,18 @@ end_write_group(JOIN *join, JOIN_TAB *join_tab __attribute__((unused)),
                        join->sum_funcs_end[send_group_parts]);
 	if (join->having && join->having->val_int() == 0)
           error= -1;
-        else if ((error=table->file->write_row(table->record[0])))
+        else if ((error= table->file->write_row(table->record[0])))
 	{
 	  if (create_myisam_from_heap(join->thd, table,
 				      &join->tmp_table_param,
 				      error, 0))
 	    DBUG_RETURN(-1);		       
         }
-        if (join->rollup.state != ROLLUP::STATE_NONE && error <= 0)
+        if (join->rollup.state != ROLLUP::STATE_NONE)
 	{
 	  if (join->rollup_write_data((uint) (idx+1), table))
-	    error= 1;
+	    DBUG_RETURN(-1);
 	}
-	if (error > 0)
-	  DBUG_RETURN(-1);	  
 	if (end_of_records)
 	  DBUG_RETURN(0);
       }
@@ -12685,17 +12683,21 @@ bool JOIN::rollup_make_fields(List<Item> &fields_arg, List<Item> &sel_fields,
       {
 	/* Check if this is something that is part of this group by */
 	ORDER *group_tmp;
-	for (group_tmp= start_group, i-- ;
+	for (group_tmp= start_group, i= pos ;
              group_tmp ; group_tmp= group_tmp->next, i++)
 	{
 	  if (*group_tmp->item == item)
 	  {
+            Item_null_result *null_item;
 	    /*
 	      This is an element that is used by the GROUP BY and should be
 	      set to NULL in this level
 	    */
 	    item->maybe_null= 1;		// Value will be null sometimes
-            Item_null_result *null_item= rollup.null_items[i];
+            null_item= rollup.null_items[i];
+            DBUG_ASSERT(null_item->result_field == 0 ||
+                        null_item->result_field ==
+                        ((Item_field *) item)->result_field);
             null_item->result_field= ((Item_field *) item)->result_field;
             item= null_item;
 	    break;

sql/sql_string.h

@@ -84,6 +84,7 @@ class String
   inline char& operator [] (uint32 i) const { return Ptr[i]; }
   inline void length(uint32 len) { str_length=len ; }
   inline bool is_empty() { return (str_length == 0); }
+  inline void mark_as_const() { Alloced_length= 0;}
   inline const char *ptr() const { return Ptr; }
   inline char *c_ptr()
   {
@@ -205,10 +206,6 @@ class String
       }
     }
   }
-  inline void shrink_to_length()
-  {
-    Alloced_length= str_length;
-  }
   bool is_alloced() { return alloced; }
   inline String& operator = (const String &s)
   {