Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()

The problem was that when one used String::alloc() to allocate a string,
the String ensures that there is space for an extra NULL byte in the
buffer and if not, reallocates the string. This is a problem with the
String::set_int() that calls alloc(21), which forces extra
malloc/free calls to happen.

- We do not anymore re-allocate String if alloc() is called with the
  Allocated_length. This reduces number of malloc() allocations,
  especially one big re-allocation in Protocol::send_result_Set_metadata()
  for almost every query that produced a result to the connnected client.
- Avoid extra mallocs when using LONGLONG_BUFFER_SIZE
  This can now be done as alloc() doesn't increase buffers if new length is
  not bigger than old one.
- c_ptr() is redesigned to be safer (but a bit longer) than before.
- Remove wrong usage of c_ptr_quick()
  c_ptr_quick() was used in many cases to get the pointer to the used
  buffer, even when it didn't need to be \0 terminated. In this case
  ptr() is a better substitute.
  Another problem with c_ptr_quick() is that it did not guarantee that
  the string would be \0 terminated.
- item_val_str(), an API function not used currently by the server,
  now always returns a null terminated string (before it didn't always
  do that).
- Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old
  mixed usage of performance keys caused assert's when String buffers
  where shrunk.
- Binary_string::shrink() is simplifed
- Fixed bug in String(const char *str, size_t len, CHARSET_INFO *cs) that
  used Binary_string((char *) str, len) instead of Binary_string(str,len).
- Changed argument to String() creations and String.set() functions to use
  'const char*' instead of 'char*'. This ensures that Alloced_length is
  not set, which gives safety against someone trying to change the
  original string. This also would allow us to use !Alloced_length in
  c_ptr() if needed.
- Changed string_ptr_cmp() to use memcmp() instead of c_ptr() to avoid
  a possible malloc during string comparision.

15 files changed, 124 insertions, 72 deletions

diff --git a/sql/field_conv.cc b/sql/field_conv.cc
index ff6d60e7626..47d63fe4645 100644
--- a/sql/field_conv.cc
+++ b/sql/field_conv.cc
@@ -369,7 +369,7 @@ void Field::do_field_string(Copy_field *copy)
   res.length(0U);
 
   copy->from_field->val_str(&res);
-  copy->to_field->store(res.c_ptr_quick(), res.length(), res.charset());
+  copy->to_field->store(res.ptr(), res.length(), res.charset());
 }
 
 
@@ -389,10 +389,10 @@ static void do_field_varbinary_pre50(Copy_field *copy)
   copy->from_field->val_str(&copy->tmp);
 
   /* Use the same function as in 4.1 to trim trailing spaces */
-  size_t length= my_lengthsp_8bit(&my_charset_bin, copy->tmp.c_ptr_quick(),
+  size_t length= my_lengthsp_8bit(&my_charset_bin, copy->tmp.ptr(),
                                 copy->from_field->field_length);
 
-  copy->to_field->store(copy->tmp.c_ptr_quick(), length,
+  copy->to_field->store(copy->tmp.ptr(), length,
                         copy->tmp.charset());
 }
 
diff --git a/sql/item.cc b/sql/item.cc
index 438c46894d5..5e0f2f72440 100644
--- a/sql/item.cc
+++ b/sql/item.cc
@@ -4599,7 +4599,7 @@ const String *Item_param::value_query_val_str(THD *thd, String *str) const
         break;
       }
       DBUG_ASSERT(str->length() <= typelen);
-      buf= str->c_ptr_quick();
+      buf= (char*) str->ptr();
       ptr= buf + str->length();
       *ptr++= '\'';
       ptr+= (uint) my_TIME_to_str(&value.time, ptr, decimals);
@@ -4705,7 +4705,7 @@ Item *Item_param::value_clone_item(THD *thd)
     return 0; // Should create Item_decimal. See MDEV-11361.
   case STRING_RESULT:
     return new (mem_root) Item_string(thd, name,
-                                      Lex_cstring(value.m_string.c_ptr_quick(),
+                                      Lex_cstring(value.m_string.ptr(),
                                                   value.m_string.length()),
                                       value.m_string.charset(),
                                       collation.derivation,
diff --git a/sql/partition_info.cc b/sql/partition_info.cc
index 871411cf6c4..fd92e437cac 100644
--- a/sql/partition_info.cc
+++ b/sql/partition_info.cc
@@ -126,7 +126,7 @@ partition_info *partition_info::get_clone(THD *thd)
 /**
   Mark named [sub]partition to be used/locked.
 
-  @param part_name  Partition name to match.
+  @param part_name  Partition name to match.  Must be \0 terminated!
   @param length     Partition name length.
 
   @return Success if partition found
@@ -140,7 +140,10 @@ bool partition_info::add_named_partition(const char *part_name, size_t length)
   PART_NAME_DEF *part_def;
   Partition_share *part_share;
   DBUG_ENTER("partition_info::add_named_partition");
-  DBUG_ASSERT(table && table->s && table->s->ha_share);
+  DBUG_ASSERT(part_name[length] == 0);
+  DBUG_ASSERT(table);
+  DBUG_ASSERT(table->s);
+  DBUG_ASSERT(table->s->ha_share);
   part_share= static_cast<Partition_share*>((table->s->ha_share));
   DBUG_ASSERT(part_share->partition_name_hash_initialized);
   part_name_hash= &part_share->partition_name_hash;
@@ -172,9 +175,9 @@ bool partition_info::add_named_partition(const char *part_name, size_t length)
     else
       bitmap_set_bit(&read_partitions, part_def->part_id);
   }
-  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
+  DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
                       part_def->part_id, part_def->is_subpart,
-                      part_name));
+                      length, part_name));
   DBUG_RETURN(false);
 }
 
diff --git a/sql/sql_analyze_stmt.cc b/sql/sql_analyze_stmt.cc
index 2f87b9b0d40..8df2f76aeaf 100644
--- a/sql/sql_analyze_stmt.cc
+++ b/sql/sql_analyze_stmt.cc
@@ -81,7 +81,7 @@ void Filesort_tracker::print_json_members(Json_writer *writer)
   }
 
   get_data_format(&str);
-  writer->add_member("r_sort_mode").add_str(str.c_ptr(), str.length());
+  writer->add_member("r_sort_mode").add_str(str.ptr(), str.length());
 }
 
 void Filesort_tracker::get_data_format(String *str)
diff --git a/sql/sql_help.cc b/sql/sql_help.cc
index 01c47bc2b21..8ce0f724e55 100644
--- a/sql/sql_help.cc
+++ b/sql/sql_help.cc
@@ -541,7 +541,12 @@ extern "C" int string_ptr_cmp(const void* ptr1, const void* ptr2)
 {
   String *str1= *(String**)ptr1;
   String *str2= *(String**)ptr2;
-  return strcmp(str1->c_ptr(),str2->c_ptr());
+  uint length1= str1->length();
+  uint length2= str2->length();
+  int tmp= memcmp(str1->ptr(),str2->ptr(), MY_MIN(length1, length2));
+  if (tmp)
+    return tmp;
+  return (int) length2 - (int) length1;
 }
 
 /*
diff --git a/sql/sql_plugin.cc b/sql/sql_plugin.cc
index 02ae08c910c..195267b9abc 100644
--- a/sql/sql_plugin.cc
+++ b/sql/sql_plugin.cc
@@ -404,12 +404,16 @@ static int item_value_type(struct st_mysql_value *value)
 static const char *item_val_str(struct st_mysql_value *value,
                                 char *buffer, int *length)
 {
-  String str(buffer, *length, system_charset_info), *res;
+  size_t org_length= *length;
+  String str(buffer, org_length, system_charset_info), *res;
   if (!(res= ((st_item_value_holder*)value)->item->val_str(&str)))
     return NULL;
   *length= res->length();
-  if (res->c_ptr_quick() == buffer)
+  if (res->ptr() == buffer && res->length() < org_length)
+  {
+    buffer[res->length()]= 0;
     return buffer;
+  }
 
   /*
     Lets be nice and create a temporary string since the
diff --git a/sql/sql_repl.cc b/sql/sql_repl.cc
index ff2faca5ecf..49fd29a6a31 100644
--- a/sql/sql_repl.cc
+++ b/sql/sql_repl.cc
@@ -2153,7 +2153,7 @@ static int init_binlog_sender(binlog_send_info *info,
         "Start binlog_dump to slave_server(%lu), pos(%s, %lu), "
         "using_gtid(%d), gtid('%s')", thd->variables.server_id,
         log_ident, (ulong)*pos, info->using_gtid_state,
-        connect_gtid_state.c_ptr_quick());
+        connect_gtid_state.c_ptr_safe());
   }
 
 #ifndef DBUG_OFF
@@ -2176,7 +2176,7 @@ static int init_binlog_sender(binlog_send_info *info,
   const char *name=search_file_name;
   if (info->using_gtid_state)
   {
-    if (info->gtid_state.load(connect_gtid_state.c_ptr_quick(),
+    if (info->gtid_state.load(connect_gtid_state.ptr(),
                              connect_gtid_state.length()))
     {
       info->errmsg= "Out of memory or malformed slave request when obtaining "
@@ -2185,7 +2185,7 @@ static int init_binlog_sender(binlog_send_info *info,
       return 1;
     }
     if (info->until_gtid_state &&
-        info->until_gtid_state->load(slave_until_gtid_str.c_ptr_quick(),
+        info->until_gtid_state->load(slave_until_gtid_str.ptr(),
                                     slave_until_gtid_str.length()))
     {
       info->errmsg= "Out of memory or malformed slave request when "
diff --git a/sql/sql_select.cc b/sql/sql_select.cc
index 2f317f58da6..14351268af6 100644
--- a/sql/sql_select.cc
+++ b/sql/sql_select.cc
@@ -27860,7 +27860,7 @@ void TABLE_LIST::print(THD *thd, table_map eliminated_tables, String *str,
         for (i= 1; i <= num_parts; i++)
         {
           String *name= name_it++;
-          append_identifier(thd, str, name->c_ptr(), name->length());
+          append_identifier(thd, str, name->ptr(), name->length());
           if (i != num_parts)
             str->append(',');
         }
diff --git a/sql/sql_show.cc b/sql/sql_show.cc
index 62942e29344..617caf12aa3 100644
--- a/sql/sql_show.cc
+++ b/sql/sql_show.cc
@@ -10121,7 +10121,7 @@ char *thd_get_error_context_description(THD *thd, char *buffer,
   */
   DBUG_ASSERT(buffer != NULL);
   length= MY_MIN(str.length(), length-1);
-  memcpy(buffer, str.c_ptr_quick(), length);
+  memcpy(buffer, str.ptr(), length);
   /* Make sure that the new string is null terminated */
   buffer[length]= '\0';
   return buffer;
diff --git a/sql/sql_string.cc b/sql/sql_string.cc
index f2a0f55aec8..aca9ede9000 100644
--- a/sql/sql_string.cc
+++ b/sql/sql_string.cc
@@ -41,7 +41,7 @@ bool Binary_string::real_alloc(size_t length)
   if (Alloced_length < arg_length)
   {
     free();
-    if (!(Ptr=(char*) my_malloc(PSI_INSTRUMENT_ME,
+    if (!(Ptr=(char*) my_malloc(STRING_PSI_MEMORY_KEY,
                                 arg_length,MYF(MY_WME | (thread_specific ?
                                                 MY_THREAD_SPECIFIC : 0)))))
       return TRUE;
@@ -55,7 +55,8 @@ bool Binary_string::real_alloc(size_t length)
 
 
 /**
-   Allocates a new buffer on the heap for this String.
+   Allocates a new buffer on the heap for this String if current buffer is
+   smaller.
 
    - If the String's internal buffer is privately owned and heap allocated,
      one of the following is performed.
@@ -70,7 +71,8 @@ bool Binary_string::real_alloc(size_t length)
      will be allocated and the string copied accoring to its length, as found
      in String::length().
  
-   For C compatibility, the new string buffer is null terminated.
+   For C compatibility, the new string buffer is null terminated if it was
+   allocated.
 
    @param alloc_length The requested string size in characters, excluding any
    null terminator.
@@ -81,9 +83,10 @@ bool Binary_string::real_alloc(size_t length)
 
    @retval true An error occurred when attempting to allocate memory.
 */
+
 bool Binary_string::realloc_raw(size_t alloc_length)
 {
-  if (Alloced_length <= alloc_length)
+  if (Alloced_length < alloc_length)
   {
     char *new_ptr;
     uint32 len= ALIGN_SIZE(alloc_length+1);
@@ -92,13 +95,13 @@ bool Binary_string::realloc_raw(size_t alloc_length)
       return TRUE;                                 /* Overflow */
     if (alloced)
     {
-      if (!(new_ptr= (char*) my_realloc(PSI_INSTRUMENT_ME, Ptr,len,
+      if (!(new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr,len,
                                         MYF(MY_WME |
                                             (thread_specific ?
                                              MY_THREAD_SPECIFIC : 0)))))
         return TRUE;				// Signal error
     }
-    else if ((new_ptr= (char*) my_malloc(PSI_INSTRUMENT_ME, len,
+    else if ((new_ptr= (char*) my_malloc(STRING_PSI_MEMORY_KEY, len,
                                          MYF(MY_WME |
                                              (thread_specific ?
                                               MY_THREAD_SPECIFIC : 0)))))
@@ -118,9 +121,14 @@ bool Binary_string::realloc_raw(size_t alloc_length)
   return FALSE;
 }
 
+
 bool String::set_int(longlong num, bool unsigned_flag, CHARSET_INFO *cs)
 {
-  uint l=20*cs->mbmaxlen+1;
+  /*
+    This allocates a few bytes extra in the unlikely case that cs->mb_maxlen
+    > 1, but we can live with that
+  */
+  uint l= LONGLONG_BUFFER_SIZE * cs->mbmaxlen;
   int base= unsigned_flag ? 10 : -10;
 
   if (alloc(l))
@@ -235,7 +243,7 @@ bool Binary_string::copy()
 */
 bool Binary_string::copy(const Binary_string &str)
 {
-  if (alloc(str.str_length))
+  if (alloc(str.str_length+1))
     return TRUE;
   if ((str_length=str.str_length))
     bmove(Ptr,str.Ptr,str_length);		// May be overlapping
@@ -246,7 +254,7 @@ bool Binary_string::copy(const Binary_string &str)
 bool Binary_string::copy(const char *str, size_t arg_length)
 {
   DBUG_ASSERT(arg_length < UINT_MAX32);
-  if (alloc(arg_length))
+  if (alloc(arg_length+1))
     return TRUE;
   if (Ptr == str && arg_length == uint32(str_length))
   {
@@ -272,7 +280,7 @@ bool Binary_string::copy(const char *str, size_t arg_length)
 bool Binary_string::copy_or_move(const char *str, size_t arg_length)
 {
   DBUG_ASSERT(arg_length < UINT_MAX32);
-  if (alloc(arg_length))
+  if (alloc(arg_length+1))
     return TRUE;
   if ((str_length=uint32(arg_length)))
     memmove(Ptr,str,arg_length);
@@ -1251,24 +1259,16 @@ bool String::append_semi_hex(const char *s, uint len, CHARSET_INFO *cs)
   return false;
 }
 
+
 // Shrink the buffer, but only if it is allocated on the heap.
 void Binary_string::shrink(size_t arg_length)
 {
-    if (!is_alloced())
-        return;
-    if (ALIGN_SIZE(arg_length + 1) < Alloced_length)
-    {
-        char* new_ptr;
-        if (!(new_ptr = (char*)my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length,
-            MYF(thread_specific ? MY_THREAD_SPECIFIC : 0))))
-        {
-            Alloced_length = 0;
-            real_alloc(arg_length);
-        }
-        else
-        {
-            Ptr = new_ptr;
-            Alloced_length = (uint32)arg_length;
-        }
-    }
+  if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length)
+  {
+    /* my_realloc() can't fail as new buffer is less than the original one */
+    Ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr, arg_length,
+                            MYF(thread_specific ?
+                                MY_THREAD_SPECIFIC : 0));
+    Alloced_length= (uint32) arg_length;
+  }
 }
diff --git a/sql/sql_string.h b/sql/sql_string.h
index b3eca118b63..c731f6189c8 100644
--- a/sql/sql_string.h
+++ b/sql/sql_string.h
@@ -600,25 +600,55 @@ public:
 
   inline char *c_ptr()
   {
-    DBUG_ASSERT(!alloced || !Ptr || !Alloced_length ||
-                (Alloced_length >= (str_length + 1)));
-
-    if (!Ptr || Ptr[str_length])              // Should be safe
-      (void) realloc(str_length);
+    if (unlikely(!Ptr))
+      return (char*) "";
+    /*
+      Here we assume that any buffer used to initalize String has
+      an end \0 or have at least an accessable character at end.
+      This is to handle the case of String("Hello",5) and
+      String("hello",5) efficiently.
+
+      We have two options here. To test for !Alloced_length or !alloced.
+      Using "Alloced_length" is slightly safer so that we do not read
+      from potentially unintialized memory (normally not dangerous but
+      may give warnings in valgrind), but "alloced" is safer as there
+      are less change to get memory loss from code that is using
+      String((char*), length) or String.set((char*), length) and does
+      not free things properly (and there is several places in the code
+      where this happens and it is hard to find out if any of these will call
+      c_ptr().
+    */
+    if (unlikely(!alloced && !Ptr[str_length]))
+      return Ptr;
+    if (str_length < Alloced_length)
+    {
+      Ptr[str_length]=0;
+      return Ptr;
+    }
+    (void) realloc(str_length+1);               /* This will add end \0 */
     return Ptr;
   }
+  /*
+    One should use c_ptr() instead for most cases. This will be deleted soon,
+    kept for compatiblity.
+  */
   inline char *c_ptr_quick()
   {
-    if (Ptr && str_length < Alloced_length)
-      Ptr[str_length]=0;
-    return Ptr;
+    return c_ptr_safe();
   }
+  /*
+    This is to be used only in the case when one cannot use c_ptr().
+    The cases are:
+    - When one initializes String with an external buffer and length and
+      buffer[length] could be uninitalized when c_ptr() is called.
+    - When valgrind gives warnings about uninitialized memory with c_ptr().
+  */
   inline char *c_ptr_safe()
   {
     if (Ptr && str_length < Alloced_length)
       Ptr[str_length]=0;
     else
-      (void) realloc(str_length);
+      (void) realloc(str_length + 1);
     return Ptr;
   }
 
@@ -634,7 +664,16 @@ public:
   }
   inline bool alloc(size_t arg_length)
   {
-    if (arg_length < Alloced_length)
+    /*
+      Allocate if we need more space or if we don't have p_done any
+      allocation yet (we don't want to have Ptr to be NULL for empty strings).
+
+      Note that if arg_length == Alloced_length then we don't allocate.
+      This ensures we don't do any extra allocations in protocol and String:int,
+      but the string will not be atomically null terminated if c_ptr() is not
+      called.
+    */
+    if (arg_length <= Alloced_length && Alloced_length)
       return 0;
     return real_alloc(arg_length);
   }
@@ -642,7 +681,7 @@ public:
   bool realloc_raw(size_t arg_length);
   bool realloc(size_t arg_length)
   {
-    if (realloc_raw(arg_length))
+    if (realloc_raw(arg_length+1))
       return TRUE;
     Ptr[arg_length]= 0; // This make other funcs shorter
     return FALSE;
@@ -743,7 +782,7 @@ public:
   */
   String(const char *str, size_t len, CHARSET_INFO *cs)
    :Charset(cs),
-    Binary_string((char *) str, len)
+    Binary_string(str, len)
   { }
   String(char *str, size_t len, CHARSET_INFO *cs)
    :Charset(cs),
diff --git a/sql/sql_type.cc b/sql/sql_type.cc
index 6a1ccbbbd22..8ee4572a422 100644
--- a/sql/sql_type.cc
+++ b/sql/sql_type.cc
@@ -9258,7 +9258,7 @@ Type_handler_general_purpose_int::partition_field_append_value(
                                             const
 {
   DBUG_ASSERT(item_expr->cmp_type() == INT_RESULT);
-  StringBuffer<21> tmp;
+  StringBuffer<LONGLONG_BUFFER_SIZE> tmp;
   longlong value= item_expr->val_int();
   tmp.set(value, system_charset_info);
   return str->append(tmp);
diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
index c407a395d04..d3d99ef9852 100644
--- a/sql/sql_yacc.yy
+++ b/sql/sql_yacc.yy
@@ -12077,18 +12077,18 @@ using_list:
           {
             if (unlikely(!($$= new (thd->mem_root) List<String>)))
               MYSQL_YYABORT;
-            String *s= new (thd->mem_root) String((const char *) $1.str,
-                                                    $1.length,
-                                                    system_charset_info);
+            String *s= new (thd->mem_root) String((const char*) $1.str,
+                                                  $1.length,
+                                                  system_charset_info);
             if (unlikely(unlikely(s == NULL)))
               MYSQL_YYABORT;
             $$->push_back(s, thd->mem_root);
           }
         | using_list ',' ident
           {
-            String *s= new (thd->mem_root) String((const char *) $3.str,
-                                                    $3.length,
-                                                    system_charset_info);
+            String *s= new (thd->mem_root) String((const char*) $3.str,
+                                                  $3.length,
+                                                  system_charset_info);
             if (unlikely(unlikely(s == NULL)))
               MYSQL_YYABORT;
             if (unlikely($1->push_back(s, thd->mem_root)))
@@ -14256,8 +14256,9 @@ wild_and_where:
           /* empty */ { $$= 0; }
         | LIKE remember_tok_start TEXT_STRING_sys
           {
-            Lex->wild= new (thd->mem_root) String($3.str, $3.length,
-                                                    system_charset_info);
+            Lex->wild= new (thd->mem_root) String((const char*) $3.str,
+                                                   $3.length,
+                                                   system_charset_info);
             if (unlikely(Lex->wild == NULL))
               MYSQL_YYABORT;
             $$= $2;
@@ -14931,9 +14932,9 @@ text_literal:
 text_string:
           TEXT_STRING_literal
           {
-            $$= new (thd->mem_root) String($1.str,
-                                             $1.length,
-                                             thd->variables.collation_connection);
+            $$= new (thd->mem_root) String((const char*) $1.str,
+                                           $1.length,
+                                           thd->variables.collation_connection);
             if (unlikely($$ == NULL))
               MYSQL_YYABORT;
           }
diff --git a/sql/table.cc b/sql/table.cc
index fef3982e3bd..f62b7496ce7 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -4841,7 +4841,7 @@ rename_file_ext(const char * from,const char * to,const char * ext)
 
 bool get_field(MEM_ROOT *mem, Field *field, String *res)
 {
-  char *to;
+  const char *to;
   StringBuffer<MAX_FIELD_WIDTH> str;
   bool rc;
   THD *thd= field->get_thd();
diff --git a/sql/wsrep_schema.cc b/sql/wsrep_schema.cc
index 122233b239a..1bfce7874ae 100644
--- a/sql/wsrep_schema.cc
+++ b/sql/wsrep_schema.cc
@@ -1250,7 +1250,7 @@ int Wsrep_schema::replay_transaction(THD* orig_thd,
     {
       Wsrep_schema_impl::thd_context_switch thd_context_switch(&thd, orig_thd);
 
-      ret= wsrep_apply_events(orig_thd, rli, buf.c_ptr_quick(), buf.length());
+      ret= wsrep_apply_events(orig_thd, rli, buf.ptr(), buf.length());
       if (ret)
       {
         WSREP_WARN("Wsrep_schema::replay_transaction: failed to apply fragments");
@@ -1404,7 +1404,7 @@ int Wsrep_schema::recover_sr_transactions(THD *orig_thd)
       String data_str;
 
       (void)frag_table->field[4]->val_str(&data_str);
-      wsrep::const_buffer data(data_str.c_ptr_quick(), data_str.length());
+      wsrep::const_buffer data(data_str.ptr(), data_str.length());
       wsrep::ws_meta ws_meta(gtid,
                              wsrep::stid(server_id,
                                          transaction_id,