cleanup: freshen up CREATE SERVER code

* pass LEX_STRING's from the parser, don't ignore the length only to strlen later
* init LEX::server_options only for SERVER commands, not for every statement
* don't put temporary values into a global persistent memroot

but really it's just scratching a surface

sql/sql_lex.cc

@@ -523,21 +523,6 @@ void lex_start(THD *thd)
   lex->select_lex.nest_level_base= &lex->unit;
   lex->allow_sum_func= 0;
   lex->in_sum_func= NULL;
-  /*
-    ok, there must be a better solution for this, long-term
-    I tried "bzero" in the sql_yacc.yy code, but that for
-    some reason made the values zero, even if they were set
-  */
-  lex->server_options.server_name= 0;
-  lex->server_options.server_name_length= 0;
-  lex->server_options.host= 0;
-  lex->server_options.db= 0;
-  lex->server_options.username= 0;
-  lex->server_options.password= 0;
-  lex->server_options.scheme= 0;
-  lex->server_options.socket= 0;
-  lex->server_options.owner= 0;
-  lex->server_options.port= -1;
 
   lex->is_lex_started= TRUE;
   lex->used_tables= 0;

sql/sql_lex.h

@@ -202,8 +202,13 @@ typedef Mem_root_array<ORDER*, true> Group_list_ptrs;
 typedef struct st_lex_server_options
 {
   long port;
-  uint server_name_length;
-  char *server_name, *host, *db, *username, *password, *scheme, *socket, *owner;
+  LEX_STRING server_name, host, db, username, password, scheme, socket, owner;
+  void reset(LEX_STRING name)
+  {
+    server_name= name;
+    host= db= username= password= scheme= socket= owner= null_lex_str;
+    port= -1;
+  }
 } LEX_SERVER_OPTIONS;
 
 

sql/sql_parse.cc

@@ -5481,8 +5481,8 @@ mysql_execute_command(THD *thd)
     if ((error= create_server(thd, &lex->server_options)))
     {
       DBUG_PRINT("info", ("problem creating server <%s>",
-                          lex->server_options.server_name));
-      my_error(error, MYF(0), lex->server_options.server_name);
+                          lex->server_options.server_name.str));
+      my_error(error, MYF(0), lex->server_options.server_name.str);
       break;
     }
     my_ok(thd, 1);
@@ -5500,8 +5500,8 @@ mysql_execute_command(THD *thd)
     if ((error= alter_server(thd, &lex->server_options)))
     {
       DBUG_PRINT("info", ("problem altering server <%s>",
-                          lex->server_options.server_name));
-      my_error(error, MYF(0), lex->server_options.server_name);
+                          lex->server_options.server_name.str));
+      my_error(error, MYF(0), lex->server_options.server_name.str);
       break;
     }
     my_ok(thd, 1);
@@ -5521,8 +5521,8 @@ mysql_execute_command(THD *thd)
       if (! lex->check_exists && err_code == ER_FOREIGN_SERVER_DOESNT_EXIST)
       {
         DBUG_PRINT("info", ("problem dropping server %s",
-                            lex->server_options.server_name));
-        my_error(err_code, MYF(0), lex->server_options.server_name);
+                            lex->server_options.server_name.str));
+        my_error(err_code, MYF(0), lex->server_options.server_name.str);
       }
       else
       {

sql/sql_servers.cc

@@ -64,9 +64,7 @@ static int insert_server_record_into_cache(FOREIGN_SERVER *server);
 static FOREIGN_SERVER *
 prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options);
 /* drop functions */ 
-static int delete_server_record(TABLE *table,
-                                char *server_name,
-                                size_t server_name_length);
+static int delete_server_record(TABLE *table, LEX_STRING *name);
 static int delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options);
 
 /* update functions */
@@ -81,8 +79,6 @@ static int update_server_record_in_cache(FOREIGN_SERVER *existing,
 /* utility functions */
 static void merge_server_struct(FOREIGN_SERVER *from, FOREIGN_SERVER *to);
 
-
-
 static uchar *servers_cache_get_key(FOREIGN_SERVER *server, size_t *length,
 			       my_bool not_used __attribute__((unused)))
 {
@@ -603,12 +599,10 @@ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
   int error;
   TABLE_LIST tables;
   TABLE *table;
-  LEX_STRING name= { server_options->server_name, 
-                     server_options->server_name_length };
 
   DBUG_ENTER("drop_server");
   DBUG_PRINT("info", ("server name server->server_name %s",
-                      server_options->server_name));
+                      server_options->server_name.str));
 
   tables.init_one_table("mysql", 5, "servers", 7, "servers", TL_WRITE);
 
@@ -624,12 +618,12 @@ int drop_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
     goto end;
   }
 
-  error= delete_server_record(table, name.str, name.length);
+  error= delete_server_record(table, &server_options->server_name);
 
   /* close the servers table before we call closed_cached_connection_tables */
   close_mysql_tables(thd);
 
-  if (close_cached_connection_tables(thd, &name))
+  if (close_cached_connection_tables(thd, &server_options->server_name))
   {
     push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
                         ER_UNKNOWN_ERROR, "Server connection in use");
@@ -666,19 +660,19 @@ delete_server_record_in_cache(LEX_SERVER_OPTIONS *server_options)
   FOREIGN_SERVER *server;
   DBUG_ENTER("delete_server_record_in_cache");
 
-  DBUG_PRINT("info",("trying to obtain server name %s length %d",
-                     server_options->server_name,
-                     server_options->server_name_length));
+  DBUG_PRINT("info",("trying to obtain server name %s length %zu",
+                     server_options->server_name.str,
+                     server_options->server_name.length));
 
 
   if (!(server= (FOREIGN_SERVER *)
         my_hash_search(&servers_cache,
-                       (uchar*) server_options->server_name,
-                       server_options->server_name_length)))
+                       (uchar*) server_options->server_name.str,
+                       server_options->server_name.length)))
   {
-    DBUG_PRINT("info", ("server_name %s length %d not found!",
-                        server_options->server_name,
-                        server_options->server_name_length));
+    DBUG_PRINT("info", ("server_name %s length %zu not found!",
+                        server_options->server_name.str,
+                        server_options->server_name.length));
     goto end;
   }
   /*
@@ -937,16 +931,15 @@ update_server_record(TABLE *table, FOREIGN_SERVER *server)
 */
 
 static int 
-delete_server_record(TABLE *table,
-                     char *server_name, size_t server_name_length)
+delete_server_record(TABLE *table, LEX_STRING *name)
 {
   int error;
   DBUG_ENTER("delete_server_record");
   tmp_disable_binlog(table->in_use);
   table->use_all_columns();
 
   /* set the field that's the PK to the value we're looking for */
-  table->field[0]->store(server_name, server_name_length, system_charset_info);
+  table->field[0]->store(name->str, name->length, system_charset_info);
 
   if ((error= table->file->ha_index_read_idx_map(table->record[0], 0,
                                                  (uchar *)table->field[0]->ptr,
@@ -989,13 +982,13 @@ int create_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
 
   DBUG_ENTER("create_server");
   DBUG_PRINT("info", ("server_options->server_name %s",
-                      server_options->server_name));
+                      server_options->server_name.str));
 
   mysql_rwlock_wrlock(&THR_LOCK_servers);
 
   /* hit the memory first */
-  if (my_hash_search(&servers_cache, (uchar*) server_options->server_name,
-                     server_options->server_name_length))
+  if (my_hash_search(&servers_cache, (uchar*) server_options->server_name.str,
+                     server_options->server_name.length))
     goto end;
 
 
@@ -1034,31 +1027,26 @@ int create_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
 int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
 {
   int error= ER_FOREIGN_SERVER_DOESNT_EXIST;
-  FOREIGN_SERVER *altered, *existing;
-  LEX_STRING name= { server_options->server_name, 
-                     server_options->server_name_length };
+  FOREIGN_SERVER altered, *existing;
   DBUG_ENTER("alter_server");
   DBUG_PRINT("info", ("server_options->server_name %s",
-                      server_options->server_name));
+                      server_options->server_name.str));
 
   mysql_rwlock_wrlock(&THR_LOCK_servers);
 
   if (!(existing= (FOREIGN_SERVER *) my_hash_search(&servers_cache,
-                                                    (uchar*) name.str,
-                                                    name.length)))
+                                     (uchar*) server_options->server_name.str,
+                                     server_options->server_name.length)))
     goto end;
 
-  altered= (FOREIGN_SERVER *)alloc_root(&mem,
-                                        sizeof(FOREIGN_SERVER));
-
-  prepare_server_struct_for_update(server_options, existing, altered);
+  prepare_server_struct_for_update(server_options, existing, &altered);
 
-  error= update_server(thd, existing, altered);
+  error= update_server(thd, existing, &altered);
 
   /* close the servers table before we call closed_cached_connection_tables */
   close_mysql_tables(thd);
 
-  if (close_cached_connection_tables(thd, &name))
+  if (close_cached_connection_tables(thd, &server_options->server_name))
   {
     push_warning_printf(thd, Sql_condition::WARN_LEVEL_WARN,
                         ER_UNKNOWN_ERROR, "Server connection in use");
@@ -1089,50 +1077,37 @@ int alter_server(THD *thd, LEX_SERVER_OPTIONS *server_options)
 static FOREIGN_SERVER *
 prepare_server_struct_for_insert(LEX_SERVER_OPTIONS *server_options)
 {
-  char *unset_ptr= (char*)"";
   FOREIGN_SERVER *server;
   DBUG_ENTER("prepare_server_struct");
 
   if (!(server= (FOREIGN_SERVER *)alloc_root(&mem, sizeof(FOREIGN_SERVER))))
     DBUG_RETURN(NULL); /* purecov: inspected */
 
-  /* these two MUST be set */
-  if (!(server->server_name= strdup_root(&mem, server_options->server_name)))
-    DBUG_RETURN(NULL); /* purecov: inspected */
-  server->server_name_length= server_options->server_name_length;
+#define SET_SERVER_OR_RETURN(X, DEFAULT)                        \
+  do {                                                          \
+    if (!(server->X= server_options->X.str ?                    \
+            strmake_root(&mem, server_options->X.str,           \
+                               server_options->X.length) : "")) \
+      DBUG_RETURN(NULL);                                        \
+  } while(0)
 
-  if (!(server->host= server_options->host ?
-          strdup_root(&mem, server_options->host) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
+  /* name and scheme are always set (the parser guarantees it) */
+  SET_SERVER_OR_RETURN(server_name, NULL);
+  SET_SERVER_OR_RETURN(scheme, NULL);
 
-  if (!(server->db= server_options->db ?
-          strdup_root(&mem, server_options->db) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
+  SET_SERVER_OR_RETURN(host, "");
+  SET_SERVER_OR_RETURN(db, "");
+  SET_SERVER_OR_RETURN(username, "");
+  SET_SERVER_OR_RETURN(password, "");
+  SET_SERVER_OR_RETURN(socket, "");
+  SET_SERVER_OR_RETURN(owner, "");
 
-  if (!(server->username= server_options->username ?
-          strdup_root(&mem, server_options->username) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
-
-  if (!(server->password= server_options->password ?
-          strdup_root(&mem, server_options->password) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
+  server->server_name_length= server_options->server_name.length;
 
   /* set to 0 if not specified */
   server->port= server_options->port > -1 ?
     server_options->port : 0;
 
-  if (!(server->socket= server_options->socket ?
-          strdup_root(&mem, server_options->socket) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
-
-  if (!(server->scheme= server_options->scheme ?
-          strdup_root(&mem, server_options->scheme) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
-
-  if (!(server->owner= server_options->owner ?
-          strdup_root(&mem, server_options->owner) : unset_ptr))
-    DBUG_RETURN(NULL); /* purecov: inspected */
-
   DBUG_RETURN(server);
 }
 
@@ -1156,32 +1131,30 @@ prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options,
 {
   DBUG_ENTER("prepare_server_struct_for_update");
 
-  altered->server_name= strdup_root(&mem, server_options->server_name);
-  altered->server_name_length= server_options->server_name_length;
+  altered->server_name= existing->server_name;
+  altered->server_name_length= existing->server_name_length;
   DBUG_PRINT("info", ("existing name %s altered name %s",
                       existing->server_name, altered->server_name));
 
   /*
     The logic here is this: is this value set AND is it different
     than the existing value?
   */
-  altered->host=
-    (server_options->host && (strcmp(server_options->host, existing->host))) ?
-     strdup_root(&mem, server_options->host) : 0;
-
-  altered->db=
-      (server_options->db && (strcmp(server_options->db, existing->db))) ?
-        strdup_root(&mem, server_options->db) : 0;
-
-  altered->username=
-      (server_options->username &&
-      (strcmp(server_options->username, existing->username))) ?
-        strdup_root(&mem, server_options->username) : 0;
-
-  altered->password=
-      (server_options->password &&
-      (strcmp(server_options->password, existing->password))) ?
-        strdup_root(&mem, server_options->password) : 0;
+#define SET_ALTERED(X)                                                       \
+  do {                                                                       \
+    altered->X=                                                              \
+      (server_options->X.str && strcmp(server_options->X.str, existing->X))  \
+      ? strmake_root(&mem, server_options->X.str, server_options->X.length)  \
+      : 0;                                                                   \
+  } while(0)
+
+  SET_ALTERED(host);
+  SET_ALTERED(db);
+  SET_ALTERED(username);
+  SET_ALTERED(password);
+  SET_ALTERED(socket);
+  SET_ALTERED(scheme);
+  SET_ALTERED(owner);
 
   /*
     port is initialised to -1, so if unset, it will be -1
@@ -1190,21 +1163,6 @@ prepare_server_struct_for_update(LEX_SERVER_OPTIONS *server_options,
                  server_options->port != existing->port) ?
     server_options->port : -1;
 
-  altered->socket=
-    (server_options->socket &&
-    (strcmp(server_options->socket, existing->socket))) ?
-      strdup_root(&mem, server_options->socket) : 0;
-
-  altered->scheme=
-    (server_options->scheme &&
-    (strcmp(server_options->scheme, existing->scheme))) ?
-      strdup_root(&mem, server_options->scheme) : 0;
-
-  altered->owner=
-    (server_options->owner &&
-    (strcmp(server_options->owner, existing->owner))) ?
-      strdup_root(&mem, server_options->owner) : 0;
-
   DBUG_VOID_RETURN;
 }
 
@@ -1272,13 +1230,13 @@ static FOREIGN_SERVER *clone_server(MEM_ROOT *mem, const FOREIGN_SERVER *server,
   buffer->server_name_length= server->server_name_length;
 
   /* TODO: We need to examine which of these can really be NULL */
-  buffer->db= server->db ? strdup_root(mem, server->db) : NULL;
-  buffer->scheme= server->scheme ? strdup_root(mem, server->scheme) : NULL;
-  buffer->username= server->username? strdup_root(mem, server->username): NULL;
-  buffer->password= server->password? strdup_root(mem, server->password): NULL;
-  buffer->socket= server->socket ? strdup_root(mem, server->socket) : NULL;
-  buffer->owner= server->owner ? strdup_root(mem, server->owner) : NULL;
-  buffer->host= server->host ? strdup_root(mem, server->host) : NULL;
+  buffer->db= safe_strdup_root(mem, server->db);
+  buffer->scheme= safe_strdup_root(mem, server->scheme);
+  buffer->username= safe_strdup_root(mem, server->username);
+  buffer->password= safe_strdup_root(mem, server->password);
+  buffer->socket= safe_strdup_root(mem, server->socket);
+  buffer->owner= safe_strdup_root(mem, server->owner);
+  buffer->host= safe_strdup_root(mem, server->host);
 
  DBUG_RETURN(buffer);
 }

sql/sql_servers.h

@@ -26,10 +26,10 @@ typedef struct st_mem_root MEM_ROOT;
 /* structs */
 typedef struct st_federated_server
 {
-  char *server_name;
+  const char *server_name;
   long port;
   uint server_name_length;
-  char *db, *scheme, *username, *password, *socket, *owner, *host, *sport;
+  const char *db, *scheme, *username, *password, *socket, *owner, *host, *sport;
 } FOREIGN_SERVER;
 
 /* cache handlers */