WL# 2094 Federated Storage Handler

Patch #2, all coding style changes per Kostja's review 
(as requested to keep style changes separate from functionality changes),
as well as changes to the test suit to no longer use hard-coded port 
values in foreign table creation statement


mysql-test/r/federated.result:
  Now using SLAVE_PORT as opposed to hard-coded port value
mysql-test/t/federated.test:
  - changed test to use SLAVE_MYPORT
  - made all standard SQL uppercase
  - made test 80 cols, except blob inserts of text
sql/ha_federated.cc:
  - made sure all comments and style changes were according to Kostja's 
  review 
  - combined parse_url error handling into a 'error:' block, took out 
  triple exclamation points.
  - clarified commented documentation
  - got rid of all trailing spaces

1 files changed, 250 insertions, 226 deletions

diff --git a/sql/ha_federated.cc b/sql/ha_federated.cc
index cf704f9f569..06e7a2f0aa6 100644
--- a/sql/ha_federated.cc
+++ b/sql/ha_federated.cc
@@ -21,27 +21,27 @@
   ha_federated.cc - MySQL Federated Storage Engine
   Patrick Galbraith and Brian Aker, 2004
 
-  This is a handler which uses a remote database as the data file, as 
+  This is a handler which uses a foreign database as the data file, as
   opposed to a handler like MyISAM, which uses .MYD files locally.
 
   How this handler works
   ----------------------------------
-  Normal database files are local and as such: You create a table called 
-  'users', a file such as 'users.MYD' is created. A handler reads, inserts, 
+  Normal database files are local and as such: You create a table called
+  'users', a file such as 'users.MYD' is created. A handler reads, inserts,
   deletes, updates data in this file. The data is stored in particular format,
   so to read, that data has to be parsed into fields, to write, fields have to
-  be stored in this format to write to this data file. 
+  be stored in this format to write to this data file.
 
   With MySQL Federated storage engine, there will be no local files for each
-  table's data (such as .MYD). A remote database will store the data that would
-  normally be in this file. This will necessitate the use of MySQL client API 
+  table's data (such as .MYD). A foreign database will store the data that would
+  normally be in this file. This will necessitate the use of MySQL client API
   to read, delete, update, insert this data. The data will have to be retrieve
   via an SQL call "SELECT * FROM users". Then, to read this data, it will have
-  to be retrieved via mysql_fetch_row one row at a time, then converted from 
+  to be retrieved via mysql_fetch_row one row at a time, then converted from
   the  column in this select into the format that the handler expects.
 
-  The create table will simply create the .frm file, and within the 
-  "CREATE TABLE" SQL, there SHALL be any of the following : 
+  The create table will simply create the .frm file, and within the
+  "CREATE TABLE" SQL, there SHALL be any of the following :
 
   comment=scheme://username:password@hostname:port/database/tablename
   comment=scheme://username@hostname/database/tablename
@@ -57,41 +57,41 @@
   Only 'mysql://' is supported at this release.
 
 
-  This comment connection string is necessary for the handler to be 
-  able to connect to the remote server.
+  This comment connection string is necessary for the handler to be
+  able to connect to the foreign server.
 
 
   The basic flow is this:
 
   SQL calls issues locally ->
-  mysql handler API (data in handler format) -> 
-  mysql client API (data converted to SQL calls) -> 
-  remote database -> mysql client API -> 
+  mysql handler API (data in handler format) ->
+  mysql client API (data converted to SQL calls) ->
+  foreign database -> mysql client API ->
   convert result sets (if any) to handler format ->
   handler API -> results or rows affected to local
 
   What this handler does and doesn't support
   ------------------------------------------
-  * Tables MUST be created on the remote server prior to any action on those
-    tables via the handler, first version. IMPORTANT: IF you MUST use the 
+  * Tables MUST be created on the foreign server prior to any action on those
+    tables via the handler, first version. IMPORTANT: IF you MUST use the
     federated storage engine type on the REMOTE end, MAKE SURE [ :) ] That
-    the table you connect to IS NOT a table pointing BACK to your ORIGNAL 
+    the table you connect to IS NOT a table pointing BACK to your ORIGNAL
     table! You know  and have heard the screaching of audio feedback? You
-    know putting two mirror in front of each other how the reflection 
+    know putting two mirror in front of each other how the reflection
     continues for eternity? Well, need I say more?!
   * There will not be support for transactions.
-  * There is no way for the handler to know if the database on the remote end
+  * There is no way for the handler to know if the foreign database or table
     has changed. The reason for this is that this database has to work like a
-    data file that would never be written to by anything other than the 
-    database. The integrity of the data in the local table could be breached 
-    if there was any change to the remote database.
+    data file that would never be written to by anything other than the
+    database. The integrity of the data in the local table could be breached
+    if there was any change to the foreign database.
   * Support for SELECT, INSERT, UPDATE , DELETE, indexes.
   * No ALTER TABLE, DROP TABLE or any other Data Definition Language calls.
-  * Prepared statements will not be used in the first implementation, it 
+  * Prepared statements will not be used in the first implementation, it
     remains to to be seen whether the limited subset of the client API for the
     server supports this.
-  * This uses SELECT, INSERT, UPDATE, DELETE and not HANDLER for its 
-    implementation. 
+  * This uses SELECT, INSERT, UPDATE, DELETE and not HANDLER for its
+    implementation.
   * This will not work with the query cache.
 
    Method calls
@@ -100,7 +100,7 @@
 
    (SELECT)
 
-   "SELECT * FROM foo" 
+   "SELECT * FROM foo"
     ha_federated::info
     ha_federated::scan_time:
     ha_federated::rnd_init: share->select_query SELECT * FROM foo
@@ -140,7 +140,7 @@
     ha_federated::rnd_next
     ha_federated::convert_row_to_internal_format
     ha_federated::update_row
-    
+
     <quote 3 cols, new and old data>
     Field::quote_data
     Field::quote_data
@@ -149,7 +149,7 @@
     Field::quote_data
     Field::quote_data
     </quote 3 cols, new and old data>
-    
+
     ha_federated::extra
     ha_federated::extra
     ha_federated::extra
@@ -160,53 +160,53 @@
     How do I use this handler?
     --------------------------
     First of all, you need to build this storage engine:
-    
+
       ./configure --with-federated-storage-engine
       make
 
-    Next, to use this handler, it's very simple. You must 
+    Next, to use this handler, it's very simple. You must
     have two databases running, either both on the same host, or
     on different hosts.
 
-    One the server that will be connecting to the remote 
+    One the server that will be connecting to the foreign
     host (client), you create your table as such:
 
-    CREATE TABLE test_table ( 
+    CREATE TABLE test_table (
       id     int(20) NOT NULL auto_increment,
       name   varchar(32) NOT NULL default '',
       other  int(20) NOT NULL default '0',
       PRIMARY KEY  (id),
       KEY name (name),
-      KEY other_key (other)) 
-       ENGINE="FEDERATED" 
-       DEFAULT CHARSET=latin1 
+      KEY other_key (other))
+       ENGINE="FEDERATED"
+       DEFAULT CHARSET=latin1
        COMMENT='root@127.0.0.1:9306/federated/test_federated';
 
-   Notice the "COMMENT" and "ENGINE" field? This is where you 
-   respectively set the engine type, "FEDERATED" and remote
-   host information, this being the database your 'client' database 
-   will connect to and use as the "data file". Obviously, the remote
-   database is running on port 9306, so you want to start up your other 
-   database so that it is indeed on port 9306, and your federated 
-   database on a port other than that. In my setup, I use port 5554 
-   for federated, and port 5555 for the remote.
+   Notice the "COMMENT" and "ENGINE" field? This is where you
+   respectively set the engine type, "FEDERATED" and foreign
+   host information, this being the database your 'client' database
+   will connect to and use as the "data file". Obviously, the foreign
+   database is running on port 9306, so you want to start up your other
+   database so that it is indeed on port 9306, and your federated
+   database on a port other than that. In my setup, I use port 5554
+   for federated, and port 5555 for the foreign database.
 
-   Then, on the remote database:
+   Then, on the foreign database:
 
-   CREATE TABLE test_table ( 
+   CREATE TABLE test_table (
      id     int(20) NOT NULL auto_increment,
      name   varchar(32) NOT NULL default '',
      other  int(20) NOT NULL default '0',
      PRIMARY KEY  (id),
      KEY name (name),
-     KEY other_key (other)) 
+     KEY other_key (other))
      ENGINE="<NAME>" <-- whatever you want, or not specify
      DEFAULT CHARSET=latin1 ;
 
     This table is exactly the same (and must be exactly the same),
-    except that it is not using the federated handler and does 
+    except that it is not using the federated handler and does
     not need the URL.
-    
+
 
     How to see the handler in action
     --------------------------------
@@ -214,17 +214,17 @@
     When developing this handler, I compiled the federated database with
     debugging:
 
-    ./configure --with-federated-storage-engine 
+    ./configure --with-federated-storage-engine
     --prefix=/home/mysql/mysql-build/federated/ --with-debug
 
     Once compiled, I did a 'make install' (not for the purpose of installing
     the binary, but to install all the files the binary expects to see in the
-    diretory I specified in the build with --prefix, 
-    "/home/mysql/mysql-build/federated". 
-    
-    Then, I started the remote server:
+    diretory I specified in the build with --prefix,
+    "/home/mysql/mysql-build/federated".
+
+    Then, I started the foreign server:
 
-    /usr/local/mysql/bin/mysqld_safe 
+    /usr/local/mysql/bin/mysqld_safe
     --user=mysql --log=/tmp/mysqld.5555.log -P 5555
 
     Then, I went back to the directory containing the newly compiled mysqld,
@@ -238,18 +238,18 @@
     Next, I open several windows for each:
 
     1. Tail the debug trace: tail -f /tmp/mysqld.trace|grep ha_fed
-    2. Tail the SQL calls to the remote database: tail -f /tmp/mysqld.5555.log
+    2. Tail the SQL calls to the foreign database: tail -f /tmp/mysqld.5555.log
     3. A window with a client open to the federated server on port 5554
     4. A window with a client open to the federated server on port 5555
 
-    I would create a table on the client to the remote server on port 
+    I would create a table on the client to the foreign server on port
     5555, and then to the federated server on port 5554. At this point,
     I would run whatever queries I wanted to on the federated server,
-    just always remembering that whatever changes I wanted to make on 
+    just always remembering that whatever changes I wanted to make on
     the table, or if I created new tables, that I would have to do that
-    on the remote server.
-    
-    Another thing to look for is 'show variables' to show you that you have 
+    on the foreign server.
+
+    Another thing to look for is 'show variables' to show you that you have
     support for federated handler support:
 
     show variables like '%federat%'
@@ -264,26 +264,26 @@
     Testing
     -------
 
-    There is a test for MySQL Federated Storage Handler in ./mysql-test/t, 
+    There is a test for MySQL Federated Storage Handler in ./mysql-test/t,
     federatedd.test It starts both a slave and master database using
     the same setup that the replication tests use, with the exception that
     it turns off replication, and sets replication to ignore the test tables.
-    After ensuring that you actually do have support for the federated storage 
-    handler, numerous queries/inserts/updates/deletes are run, many derived 
+    After ensuring that you actually do have support for the federated storage
+    handler, numerous queries/inserts/updates/deletes are run, many derived
     from the MyISAM tests, plus som other tests which were meant to reveal
     any issues that would be most likely to affect this handler. All tests
     should work! ;)
 
-    To run these tests, go into ./mysql-test (based in the directory you 
+    To run these tests, go into ./mysql-test (based in the directory you
     built the server in)
 
     ./mysql-test-run federatedd
-   
+
     To run the test, or if you want to run the test and have debug info:
 
     ./mysql-test-run --debug federated
 
-    This will run the test in debug mode, and you can view the trace and 
+    This will run the test in debug mode, and you can view the trace and
     log files in the ./mysql-test/var/log directory
 
     ls -l mysql-test/var/log/
@@ -307,10 +307,10 @@
 
     Of course, again, you can tail the trace log:
 
-    tail -f mysql-test/var/log/master.trace |grep ha_fed  
+    tail -f mysql-test/var/log/master.trace |grep ha_fed
 
     As well as the slave query log:
-  
+
     tail -f mysql-test/var/log/slave.log
 
     Files that comprise the test suit
@@ -324,7 +324,7 @@
     Other tidbits
     -------------
 
-    These were the files that were modified or created for this 
+    These were the files that were modified or created for this
     Federated handler to work:
 
     ./configure.in
@@ -342,7 +342,7 @@
     ./mysql-test/include/have_federated_db.inc
     ./sql/ha_federated.cc
     ./sql/ha_federated.h
-    
+
 */
 
 #ifdef __GNUC__
@@ -359,12 +359,11 @@ static HASH federated_open_tables;              // Hash used to track open
                                                 // tables
 pthread_mutex_t federated_mutex;                // This is the mutex we use to
                                                 // init the hash
-static int federated_init= 0;                   // Variable for checking the
+static int federated_init= FALSE;               // Variable for checking the
                                                 // init state of hash
 
-/*
-  Function we use in the creation of our hash to get key.
-*/
+/* Function we use in the creation of our hash to get key.  */
+
 static byte *federated_get_key(FEDERATED_SHARE *share, uint *length,
                                my_bool not_used __attribute__ ((unused)))
 {
@@ -426,7 +425,7 @@ bool federated_db_end()
 
   DESCRIPTION
     populates the share with information about the connection
-    to the remote database that will serve as the data source.
+    to the foreign database that will serve as the data source.
     This string must be specified (currently) in the "comment" field,
     listed in the CREATE TABLE statement.
 
@@ -451,26 +450,25 @@ bool federated_db_end()
     1  failure, wrong string format
 
 */
+
 static int parse_url(FEDERATED_SHARE *share, TABLE *table,
                      uint table_create_flag)
 {
   DBUG_ENTER("ha_federated::parse_url");
 
   share->port= 0;
-  uint error_num= table_create_flag ? ER_CANT_CREATE_TABLE :
-    ER_CONNECT_TO_MASTER;
+  uint error_num= (table_create_flag ? ER_CANT_CREATE_TABLE :
+                   ER_CONNECT_TO_MASTER);
 
   share->scheme= my_strdup(table->s->comment, MYF(0));
 
   if ((share->username= strstr(share->scheme, "://")))
   {
     share->scheme[share->username - share->scheme]= '\0';
+
     if (strcmp(share->scheme, "mysql") != 0)
-    {
-      my_error(error_num, MYF(0),
-               "ERROR: federated handler only supports remote 'mysql://' database");
-      DBUG_RETURN(1);
-    }
+      goto error;
+
     share->username+= 3;
 
     if ((share->hostname= strchr(share->username, '@')))
@@ -483,19 +481,13 @@ static int parse_url(FEDERATED_SHARE *share, TABLE *table,
         share->username[share->password - share->username]= '\0';
         share->password++;
         share->username= share->username;
-        /* 
-          make sure there isn't an extra / or @
-        */
+        /* make sure there isn't an extra / or @ */
         if ((strchr(share->password, '/') || strchr(share->hostname, '@')))
-        {
-          my_error(error_num, MYF(0),
-                   "this connection string is not in the correct format!!!\n");
-          DBUG_RETURN(1);
-        }
-        /* 
-           Found that if the string is:
-           user:@hostname:port/database/table 
-           Then password is a null string, so set to NULL 
+          goto error;
+        /*
+          Found that if the string is:
+          user:@hostname:port/database/table
+          Then password is a null string, so set to NULL
         */
         if ((share->password[0] == '\0'))
           share->password= NULL;
@@ -503,15 +495,9 @@ static int parse_url(FEDERATED_SHARE *share, TABLE *table,
       else
         share->username= share->username;
 
-      /*
-        make sure there isn't an extra / or @
-      */
+      /* make sure there isn't an extra / or @ */
       if ((strchr(share->username, '/')) || (strchr(share->hostname, '@')))
-      {
-        my_error(error_num, MYF(0),
-                 "this connection string is not in the correct format!!!\n");
-        DBUG_RETURN(1);
-      }
+        goto error;
 
       if ((share->database= strchr(share->hostname, '/')))
       {
@@ -534,27 +520,14 @@ static int parse_url(FEDERATED_SHARE *share, TABLE *table,
           share->table_base_name++;
         }
         else
-        {
-          my_error(error_num, MYF(0),
-                   "this connection string is not in the correct format!!!\n");
-          DBUG_RETURN(1);
-        }
+          goto error;
       }
       else
-      {
-        my_error(error_num, MYF(0),
-                 "this connection string is not in the correct format!!!\n");
-        DBUG_RETURN(1);
-      }
-      /*
-        make sure there's not an extra /
-      */
+        goto error;
+      /* make sure there's not an extra / */
       if ((strchr(share->table_base_name, '/')))
-      {
-        my_error(error_num, MYF(0),
-                 "this connection string is not in the correct format!!!\n");
-        DBUG_RETURN(1);
-      }
+        goto error;
+
       if (share->hostname[0] == '\0')
         share->hostname= NULL;
 
@@ -568,45 +541,45 @@ static int parse_url(FEDERATED_SHARE *share, TABLE *table,
 
       DBUG_PRINT("ha_federated::parse_url",
                  ("scheme %s username %s password %s \
-                  hostname %s port %d database %s tablename %s\n", 
-                  share->scheme, share->username, share->password, 
+                  hostname %s port %d database %s tablename %s\n",
+                  share->scheme, share->username, share->password,
                   share->hostname, share->port, share->database,
                   share->table_base_name));
     }
     else
-    {
-      my_error(error_num, MYF(0),
-               "this connection string is not in the correct format!!!\n");
-      DBUG_RETURN(1);
-    }
+      goto error;
   }
   else
-  {
+    goto error;
+
+  DBUG_RETURN(0);
+
+error:
     my_error(error_num, MYF(0),
-             "this connection string is not in the correct format!!!\n");
+             "this connection string is not in the correct format!\n");
     DBUG_RETURN(1);
-  }
-  DBUG_RETURN(0);
+
 }
 
-/* 
+/*
   Convert MySQL result set row to handler internal format
 
   SYNOPSIS
     convert_row_to_internal_format()
-      record    Byte pointer to record 
+      record    Byte pointer to record
       row       MySQL result set row from fetchrow()
 
   DESCRIPTION
-    This method simply iterates through a row returned via fetchrow with 
+    This method simply iterates through a row returned via fetchrow with
     values from a successful SELECT , and then stores each column's value
     in the field object via the field object pointer (pointing to the table's
-    array of field object pointers). This is how the handler needs the data 
+    array of field object pointers). This is how the handler needs the data
     to be stored to then return results back to the user
 
   RETURN VALUE
-    0   After fields have had field values stored from record  
+    0   After fields have had field values stored from record
  */
+
 uint ha_federated::convert_row_to_internal_format(byte *record, MYSQL_ROW row)
 {
   ulong *lengths;
@@ -631,6 +604,28 @@ uint ha_federated::convert_row_to_internal_format(byte *record, MYSQL_ROW row)
   DBUG_RETURN(0);
 }
 
+/*
+  Create a WHERE clause based off of values in keys
+  Note: This code was inspired by key_copy from key.cc
+
+  SYNOPSIS
+    create_where_from_key ()
+      to          String object to store WHERE clause
+      key_info    KEY struct pointer
+      key         byte pointer containing key
+      key_length  length of key
+
+  DESCRIPTION
+    Using iteration through all the keys via a KEY_PART_INFO pointer,
+    This method 'extracts' the value of each key in the byte pointer
+    *key, and for each key found, constructs an appropriate WHERE clause
+
+  RETURN VALUE
+    0   After all keys have been accounted for to create the WHERE clause
+    1   No keys found
+
+  */
+
 bool ha_federated::create_where_from_key(String *to, KEY *key_info,
                                          const byte *key, uint key_length)
 {
@@ -644,9 +639,6 @@ bool ha_federated::create_where_from_key(String *to, KEY *key_info,
   {
     Field *field= key_part->field;
     needs_quotes= field->needs_quotes();
-    //bool needs_quotes= type_quote(field->type());
-    DBUG_PRINT("ha_federated::create_where_from_key",
-               ("key name %s type %d", field->field_name, field->type()));
     uint length= key_part->length;
 
     if (second_loop++ && to->append(" AND ", 5))
@@ -675,7 +667,7 @@ bool ha_federated::create_where_from_key(String *to, KEY *key_info,
       DBUG_RETURN(1);
     if (key_part->type == HA_KEYTYPE_BIT)
     {
-      /* This is can be threated as a hex string */
+      /* This is can be treated as a hex string */
       Field_bit *field= (Field_bit *) (key_part->field);
       char buff[64 + 2], *ptr;
       byte *end= (byte*)(key)+length;
@@ -757,6 +749,7 @@ bool ha_federated::create_where_from_key(String *to, KEY *key_info,
   pass to each federated handler. Do you have to have one of these? Well, you
   have pieces that are used for locking, and they are needed to function.
 */
+
 static FEDERATED_SHARE *get_share(const char *table_name, TABLE *table)
 {
   FEDERATED_SHARE *share;
@@ -767,17 +760,14 @@ static FEDERATED_SHARE *get_share(const char *table_name, TABLE *table)
   uint table_name_length, table_base_name_length;
   char *tmp_table_name, *tmp_table_base_name, *table_base_name, *select_query;
 
-  /* 
-    share->table_name has the file location - we want the actual table's
-    name!
-  */
+  /* share->table_name has the file location - we want the table's name!  */
   table_base_name= (char*) table->s->table_name;
   DBUG_PRINT("ha_federated::get_share", ("table_name %s", table_base_name));
   /*
-     So why does this exist? There is no way currently to init a storage engine.
-     Innodb and BDB both have modifications to the server to allow them to
-     do this. Since you will not want to do this, this is probably the next
-     best method.
+    So why does this exist? There is no way currently to init a storage engine.
+    Innodb and BDB both have modifications to the server to allow them to
+    do this. Since you will not want to do this, this is probably the next
+    best method.
   */
   pthread_mutex_lock(&federated_mutex);
   table_name_length= (uint) strlen(table_name);
@@ -838,10 +828,11 @@ error:
 
 
 /*
-  Free lock controls. We call this whenever we close a table. 
+  Free lock controls. We call this whenever we close a table.
   If the table had the last reference to the share then we
   free memory associated with it.
 */
+
 static int free_share(FEDERATED_SHARE *share)
 {
   pthread_mutex_lock(&federated_mutex);
@@ -864,10 +855,11 @@ static int free_share(FEDERATED_SHARE *share)
 
 /*
   If frm_error() is called then we will use this to to find out
-  what file extentions exist for the storage engine. This is 
-  also used by the default rename_table and delete_table method 
+  what file extentions exist for the storage engine. This is
+  also used by the default rename_table and delete_table method
   in handler.cc.
 */
+
 const char **ha_federated::bas_ext() const
 {
   static const char *ext[]=
@@ -888,16 +880,17 @@ const char **ha_federated::bas_ext() const
   all tables by calling ha_open() which then calls the handler
   specific open().
 */
+
 int ha_federated::open(const char *name, int mode, uint test_if_locked)
 {
-  DBUG_ENTER("ha_federated::open");
   int rc;
+  DBUG_ENTER("ha_federated::open");
 
   if (!(share= get_share(name, table)))
     DBUG_RETURN(1);
   thr_lock_data_init(&share->lock, &lock, NULL);
 
-  /* Connect to remote database mysql_real_connect() */
+  /* Connect to foreign database mysql_real_connect() */
   mysql= mysql_init(0);
   DBUG_PRINT("ha_federated::open", ("hostname %s", share->hostname));
   DBUG_PRINT("ha_federated::open", ("username %s", share->username));
@@ -929,6 +922,7 @@ int ha_federated::open(const char *name, int mode, uint test_if_locked)
   myisam table.
   For sql_base.cc look at close_data_tables().
 */
+
 int ha_federated::close(void)
 {
   DBUG_ENTER("ha_federated::close");
@@ -949,7 +943,7 @@ int ha_federated::close(void)
 
 /*
 
-  Checks if a field in a record is SQL NULL. 
+  Checks if a field in a record is SQL NULL.
 
   SYNOPSIS
     field_in_record_is_null()
@@ -959,12 +953,13 @@ int ha_federated::close(void)
 
     DESCRIPTION
       This method uses the record format information in table to track
-      the null bit in record. 
+      the null bit in record.
 
     RETURN VALUE
       1    if NULL
-      0    otherwise 
+      0    otherwise
 */
+
 inline uint field_in_record_is_null(TABLE *table,
                                     Field *field,
                                     char *record)
@@ -996,6 +991,7 @@ inline uint field_in_record_is_null(TABLE *table,
   Called from item_sum.cc, item_sum.cc, sql_acl.cc, sql_insert.cc,
   sql_insert.cc, sql_select.cc, sql_table.cc, sql_udf.cc, and sql_update.cc.
 */
+
 int ha_federated::write_row(byte *buf)
 {
   uint x= 0, num_fields= 0;
@@ -1026,9 +1022,9 @@ int ha_federated::write_row(byte *buf)
     table->timestamp_field->set_time();
 
   /*
-     get the current query id - the fields that we add to the insert
-     statement to send to the remote will not be appended unless they match
-     this query id
+    get the current query id - the fields that we add to the insert
+    statement to send to the foreign will not be appended unless they match
+    this query id
   */
   current_query_id= table->in_use->query_id;
   DBUG_PRINT("ha_federated::write_row", ("current query id %d",
@@ -1041,10 +1037,10 @@ int ha_federated::write_row(byte *buf)
   insert_string.append(" (");
   values_string.append(" VALUES (");
 
-  /* 
-     Even if one field is different, all_fields_same_query_id can't remain
-     0 if it remains 0, then that means no fields were specified in the query
-     such as in the case of INSERT INTO table VALUES (val1, val2, valN)
+  /*
+    Even if one field is different, all_fields_same_query_id can't remain
+    0 if it remains 0, then that means no fields were specified in the query
+    such as in the case of INSERT INTO table VALUES (val1, val2, valN)
   */
   for (field= table->field; *field; field++, x++)
   {
@@ -1054,8 +1050,8 @@ int ha_federated::write_row(byte *buf)
     tmp_query_id= (*field)->query_id;
   }
   /*
-     loop through the field pointer array, add any fields to both the values
-     list and the fields list that match the current query id
+    loop through the field pointer array, add any fields to both the values
+    list and the fields list that match the current query id
   */
   for (field= table->field; *field; field++, x++)
   {
@@ -1101,17 +1097,17 @@ int ha_federated::write_row(byte *buf)
   }
 
   /*
-     chop of the trailing comma, or if there were no fields, a '('
-     So, "INSERT INTO foo (" becomes "INSERT INTO foo "
-     or, with fields, "INSERT INTO foo (field1, field2," becomes
-     "INSERT INTO foo (field1, field2"
+    chop of the trailing comma, or if there were no fields, a '('
+    So, "INSERT INTO foo (" becomes "INSERT INTO foo "
+    or, with fields, "INSERT INTO foo (field1, field2," becomes
+    "INSERT INTO foo (field1, field2"
   */
   insert_string.chop();
 
   /*
-     if there were no fields, we don't want to add a closing paren
-     AND, we don't want to chop off the last char '('
-     insert will be "INSERT INTO t1 VALUES ();"
+    if there were no fields, we don't want to add a closing paren
+    AND, we don't want to chop off the last char '('
+    insert will be "INSERT INTO t1 VALUES ();"
   */
   DBUG_PRINT("ha_federated::write_row", ("x %d  num fields %d", x, num_fields));
   if (num_fields > 0)
@@ -1154,6 +1150,7 @@ int ha_federated::write_row(byte *buf)
 
   Called from sql_select.cc, sql_acl.cc, sql_update.cc, and sql_insert.cc.
 */
+
 int ha_federated::update_row(const byte *old_data, byte *new_data)
 {
   uint x= 0;
@@ -1162,9 +1159,7 @@ int ha_federated::update_row(const byte *old_data, byte *new_data)
   char old_field_value_buffer[IO_SIZE], new_field_value_buffer[IO_SIZE];
   char update_buffer[IO_SIZE], where_buffer[IO_SIZE];
 
-  /*
-    stores the value to be replaced of the field were are updating 
-  */
+  /* stores the value to be replaced of the field were are updating */
   String old_field_value(old_field_value_buffer, sizeof(old_field_value_buffer),
                          &my_charset_bin);
   old_field_value.length(0);
@@ -1175,14 +1170,14 @@ int ha_federated::update_row(const byte *old_data, byte *new_data)
   /* stores the update query */
   String update_string(update_buffer, sizeof(update_buffer), &my_charset_bin);
   update_string.length(0);
-  /* stores the WHERE clause */ 
+  /* stores the WHERE clause */
   String where_string(where_buffer, sizeof(where_buffer), &my_charset_bin);
   where_string.length(0);
 
   DBUG_ENTER("ha_federated::update_row");
 
 
-  has_a_primary_key= table->s->primary_key == 0 ? 1 : 0;
+  has_a_primary_key= (table->s->primary_key == 0 ? 1 : 0);
   primary_key_field_num= has_a_primary_key ?
     table->key_info[table->s->primary_key].key_part->fieldnr - 1 : -1;
   if (has_a_primary_key)
@@ -1193,23 +1188,23 @@ int ha_federated::update_row(const byte *old_data, byte *new_data)
   update_string.append(" SET ");
 
 /*
-   In this loop, we want to match column names to values being inserted
-   (while building INSERT statement).
+  In this loop, we want to match column names to values being inserted
+  (while building INSERT statement).
 
-   Iterate through table->field (new data) and share->old_filed (old_data) 
-   using the same index to created an SQL UPDATE statement, new data is 
-   used to create SET field=value and old data is used to create WHERE 
-   field=oldvalue
+  Iterate through table->field (new data) and share->old_filed (old_data)
+  using the same index to created an SQL UPDATE statement, new data is
+  used to create SET field=value and old data is used to create WHERE
+  field=oldvalue
  */
 
   for (Field **field= table->field; *field; field++, x++)
   {
     /*
-       In all of these tests for 'has_a_primary_key', what I'm trying to
-       accomplish is to only use the primary key in the WHERE clause if the
-       table has a primary key, as opposed to a table without a primary key
-       in which case we have to use all the fields to create a WHERE clause
-       using the old/current values, as well as adding a LIMIT statement
+      In all of these tests for 'has_a_primary_key', what I'm trying to
+      accomplish is to only use the primary key in the WHERE clause if the
+      table has a primary key, as opposed to a table without a primary key
+      in which case we have to use all the fields to create a WHERE clause
+      using the old/current values, as well as adding a LIMIT statement
     */
     if (has_a_primary_key)
     {
@@ -1226,7 +1221,7 @@ int ha_federated::update_row(const byte *old_data, byte *new_data)
       new_field_value.append("NULL");
     else
     {
-      // otherwise =
+      /* otherwise = */
       (*field)->val_str(&new_field_value);
       (*field)->quote_data(&new_field_value);
 
@@ -1299,10 +1294,11 @@ int ha_federated::update_row(const byte *old_data, byte *new_data)
   ORDER BY clauses can be used.
 
   Called in sql_acl.cc and sql_udf.cc to manage internal table information.
-  Called in sql_delete.cc, sql_insert.cc, and sql_select.cc. In sql_select 
+  Called in sql_delete.cc, sql_insert.cc, and sql_select.cc. In sql_select
   it is used for removing duplicates while in insert it is used for REPLACE
   calls.
 */
+
 int ha_federated::delete_row(const byte *buf)
 {
   uint x= 0;
@@ -1362,6 +1358,7 @@ int ha_federated::delete_row(const byte *buf)
   index. This method, which is called in the case of an SQL statement having
   a WHERE clause on a non-primary key index, simply calls index_read_idx.
 */
+
 int ha_federated::index_read(byte *buf, const byte *key,
                              uint key_len __attribute__ ((unused)),
                              enum ha_rkey_function find_flag
@@ -1377,9 +1374,10 @@ int ha_federated::index_read(byte *buf, const byte *key,
   row if any.  This is only used to read whole keys.
 
   This method is called via index_read in the case of a WHERE clause using
-  a regular non-primary key index, OR is called DIRECTLY when the WHERE clause 
+  a regular non-primary key index, OR is called DIRECTLY when the WHERE clause
   uses a PRIMARY KEY index.
 */
+
 int ha_federated::index_read_idx(byte *buf, uint index, const byte *key,
                                  uint key_len __attribute__ ((unused)),
                                  enum ha_rkey_function find_flag
@@ -1443,9 +1441,7 @@ int ha_federated::index_read_idx(byte *buf, uint index, const byte *key,
   DBUG_RETURN(rnd_next(buf));
 }
 
-/*
-  Initialized at each key walk (called multiple times unlike ::rnd_init())
-*/
+/* Initialized at each key walk (called multiple times unlike rnd_init()) */
 int ha_federated::index_init(uint keynr)
 {
   int error;
@@ -1456,9 +1452,7 @@ int ha_federated::index_init(uint keynr)
   DBUG_RETURN(0);
 }
 
-/*
-  Used to read forward through the index.
-*/
+/* Used to read forward through the index.  */
 int ha_federated::index_next(byte *buf)
 {
   DBUG_ENTER("ha_federated::index_next");
@@ -1478,20 +1472,45 @@ int ha_federated::index_next(byte *buf)
   Called from filesort.cc, records.cc, sql_handler.cc, sql_select.cc,
   sql_table.cc, and sql_update.cc.
 */
+
 int ha_federated::rnd_init(bool scan)
 {
   DBUG_ENTER("ha_federated::rnd_init");
   int num_fields, rows;
 
   /*
-     This 'scan' flag is incredibly important for this handler to work properly,
-     especially with updates that are called with indexes, because what happens 
-     without this is index_read_idx gets called, does a query using the 
-     index in a where clause, calls mysql_store_result, which then rnd_init 
-     (from sql_update.cc) is called after this, which would do a 
-     "select * from table" then a mysql_store_result, wiping out the result
-     set from index_read_idx's query, which causes the subsequent update_row
-     to update the wrong row!
+    This 'scan' flag is incredibly important for this handler to work
+    properly, especially with updates containing WHERE clauses using
+    indexed columns.
+
+    When the initial query contains a WHERE clause of the query using an
+    indexed column, it's index_read_idx that selects the exact record from
+    the foreign database.
+
+    When there is NO index in the query, either due to not having a WHERE
+    clause, or the WHERE clause is using columns that are not indexed, a
+    'full table scan' done by rnd_init, which in this situation simply means
+    a 'select * from ...' on the foreign table.
+
+    In other words, this 'scan' flag gives us the means to ensure that if
+    there is an index involved in the query, we want index_read_idx to
+    retrieve the exact record (scan flag is 0), and do not  want rnd_init
+    to do a 'full table scan' and wipe out that result set.
+
+    Prior to using this flag, the problem was most apparent with updates.
+
+    An initial query like 'UPDATE tablename SET anything = whatever WHERE
+    indexedcol = someval', index_read_idx would get called, using a query
+    constructed with a WHERE clause built from the values of index ('indexcol'
+    in this case, having a value of 'someval').  mysql_store_result would
+    then get called (this would be the result set we want to use).
+
+    After this rnd_init (from sql_update.cc) would be called, it would then
+    unecessarily call "select * from table" on the foreign table, then call
+    mysql_store_result, which would wipe out the correct previous result set
+    from the previous call of index_read_idx's that had the result set
+    containing the correct record, hence update the wrong row!
+
   */
   scan_flag= scan;
   if (scan)
@@ -1551,12 +1570,13 @@ int ha_federated::index_end(void)
   Called from filesort.cc, records.cc, sql_handler.cc, sql_select.cc,
   sql_table.cc, and sql_update.cc.
 */
+
 int ha_federated::rnd_next(byte *buf)
 {
   MYSQL_ROW row;
   DBUG_ENTER("ha_federated::rnd_next");
 
-  // Fetch a row, insert it back in a row format.
+  /* Fetch a row, insert it back in a row format. */
   current_position= result->data_cursor;
   DBUG_PRINT("ha_federated::rnd_next",
              ("current position %d", current_position));
@@ -1580,10 +1600,11 @@ int ha_federated::rnd_next(byte *buf)
 
   Called from filesort.cc, sql_select.cc, sql_delete.cc and sql_update.cc.
 */
+
 void ha_federated::position(const byte *record)
 {
   DBUG_ENTER("ha_federated::position");
-  //my_store_ptr Add seek storage
+  /* my_store_ptr Add seek storage */
   *(MYSQL_ROW_OFFSET *) ref= current_position;  // ref is always aligned
   DBUG_VOID_RETURN;
 }
@@ -1602,16 +1623,16 @@ void ha_federated::position(const byte *record)
 int ha_federated::rnd_pos(byte *buf, byte *pos)
 {
   DBUG_ENTER("ha_federated::rnd_pos");
-  /* 
-     we do not need to do any of this if there has been a scan performed already, or 
-     if this is an update and index_read_idx already has a result set in which to build
-     it's update query from
+  /*
+    we do not need to do any of this if there has been a scan performed
+    already, or if this is an update and index_read_idx already has a result
+    set in which to build it's update query from
   */
   if (scan_flag)
   {
     statistic_increment(table->in_use->status_var.ha_read_rnd_count,
                         &LOCK_status);
-    memcpy_fixed(&current_position, pos, sizeof(MYSQL_ROW_OFFSET));     // pos
+    memcpy_fixed(&current_position, pos, sizeof(MYSQL_ROW_OFFSET));  // pos
     /* is not aligned */
     result->current_row= 0;
     result->data_cursor= current_position;
@@ -1665,10 +1686,11 @@ int ha_federated::rnd_pos(byte *buf, byte *pos)
 
 */
 /* FIX: later version provide better information to the optimizer */
+
 void ha_federated::info(uint flag)
 {
   DBUG_ENTER("ha_federated::info");
-  records= 10000; // fix later 
+  records= 10000; // fix later
   DBUG_VOID_RETURN;
 }
 
@@ -1684,6 +1706,7 @@ void ha_federated::info(uint flag)
   Called from sql_select.cc by JOIN::reinit().
   Called from sql_union.cc by st_select_lex_unit::exec().
 */
+
 int ha_federated::delete_all_rows()
 {
   DBUG_ENTER("ha_federated::delete_all_rows");
@@ -1735,29 +1758,30 @@ int ha_federated::delete_all_rows()
 
   Called from lock.cc by get_lock_data().
 */
+
 THR_LOCK_DATA **ha_federated::store_lock(THD *thd,
                                          THR_LOCK_DATA **to,
                                          enum thr_lock_type lock_type)
 {
   if (lock_type != TL_IGNORE && lock.type == TL_UNLOCK)
   {
-    /* 
-       Here is where we get into the guts of a row level lock.
-       If TL_UNLOCK is set 
-       If we are not doing a LOCK TABLE or DISCARD/IMPORT
-       TABLESPACE, then allow multiple writers 
+    /*
+      Here is where we get into the guts of a row level lock.
+      If TL_UNLOCK is set
+      If we are not doing a LOCK TABLE or DISCARD/IMPORT
+      TABLESPACE, then allow multiple writers
     */
 
     if ((lock_type >= TL_WRITE_CONCURRENT_INSERT &&
          lock_type <= TL_WRITE) && !thd->in_lock_tables && !thd->tablespace_op)
       lock_type= TL_WRITE_ALLOW_WRITE;
 
-    /* 
-       In queries of type INSERT INTO t1 SELECT ... FROM t2 ...
-       MySQL would use the lock TL_READ_NO_INSERT on t2, and that
-       would conflict with TL_WRITE_ALLOW_WRITE, blocking all inserts
-       to t2. Convert the lock to a normal read lock to allow
-       concurrent inserts to t2. 
+    /*
+      In queries of type INSERT INTO t1 SELECT ... FROM t2 ...
+      MySQL would use the lock TL_READ_NO_INSERT on t2, and that
+      would conflict with TL_WRITE_ALLOW_WRITE, blocking all inserts
+      to t2. Convert the lock to a normal read lock to allow
+      concurrent inserts to t2.
     */
 
     if (lock_type == TL_READ_NO_INSERT && !thd->in_lock_tables)
@@ -1773,9 +1797,9 @@ THR_LOCK_DATA **ha_federated::store_lock(THD *thd,
 
 /*
   create() does nothing, since we have no local setup of our own.
-  FUTURE: We should potentially connect to the remote database and
-  create tables if they do not exist.
+  FUTURE: We should potentially connect to the foreign database and
 */
+
 int ha_federated::create(const char *name, TABLE *table_arg,
                          HA_CREATE_INFO *create_info)
 {