bk commit into 5.0 tree (guilhem:1.1940)

[guilhem]

Below is the list of changes that have just been committed into a local
5.0 repository of guilhem. When guilhem does a push these changes will
be propagated to the main repository and, within 24 hours after the
push, to the public repository.
For information on how to access the public repository
see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html

ChangeSet
  1.1940 05/09/09 00:14:02 guilhem@stripped +7 -0
  Putting back "mysql" exclusion in row-based repl, temporarily.
  All function calls now have a detection of errors. Removing two unneeded current_thd.
More assertions.

  wl1012-review-pending-comments.txt
    1.3 05/09/09 00:13:57 guilhem@stripped +16 -15
    updates to the TODO

  sql/sql_class.h
    1.259 05/09/09 00:13:57 guilhem@stripped +0 -3
    unneeded declaration

  sql/sql_class.cc
    1.205 05/09/09 00:13:57 guilhem@stripped +14 -9
    more error catching

  sql/slave.cc
    1.257 05/09/09 00:13:57 guilhem@stripped +1 -1
    comment

  sql/rpl_tblmap.cc
    1.5 05/09/09 00:13:57 guilhem@stripped +2 -0
    comment about error catching

  sql/log_event.cc
    1.198 05/09/09 00:13:57 guilhem@stripped +17 -12
    current_thd is not needed in binlog events, we often have thd under hand. Some
assertions, some error catching.

  sql/handler.cc
    1.192 05/09/09 00:13:57 guilhem@stripped +7 -1
    Heard from Lars that this change was causing test failures, putting it back (hopefully
not for long).

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	guilhem
# Host:	gbichot3.local
# Root:	/home/mysql_src/mysql-5.0-wl1012

--- 1.191/sql/handler.cc	2005-09-06 14:56:53 +02:00
+++ 1.192/sql/handler.cc	2005-09-09 00:13:57 +02:00
@@ -2476,10 +2476,16 @@
 #ifdef HAVE_ROW_BASED_REPLICATION
 static bool check_table_binlog_row_based(TABLE *table) 
 {
+  /*
+    Guilhem is temporarily putting back the exclusion of "mysql", even though
+    he thinks it's wrong. He'll discuss this with Lars and hopes to remove it
+    again soon.
+  */
   return
     binlog_row_based &&
     table->s->tmp_table == NO_TMP_TABLE &&
-    db_ok(table->s->db, binlog_do_db, binlog_ignore_db);
+    db_ok(table->s->db, binlog_do_db, binlog_ignore_db) &&
+    strcmp(table->s->db, "mysql") != 0;
 }
 
 template<class RowsEventT> bool binlog_log_row(TABLE* table,

--- 1.197/sql/log_event.cc	2005-09-07 16:44:33 +02:00
+++ 1.198/sql/log_event.cc	2005-09-09 00:13:57 +02:00
@@ -5036,6 +5036,7 @@
 
     if (bitmap_is_set(cols, field_ptr -  begin_ptr))
     {
+      /* Field...::unpack() cannot return 0 */
       ptr= f->unpack(f->ptr + offset, ptr);
       f->query_id= thd->query_id;
     }
@@ -5052,9 +5053,14 @@
   DBUG_ENTER("Rows_log_event::exec_event(st_relay_log_info*)");
   int error= 0;
   TABLE* const table= rli->m_table_map.get_table(m_table_id);
-  THD* const thd= current_thd;
   char const *row_start= m_rows_buf;
 
+  /*
+    'thd' has been set by exec_relay_log_event(), just before calling
+    exec_event(). We still check here to prevent future coding errors.
+  */
+  DBUG_ASSERT(rli->sql_thd == thd);
+
   if (table) 
   {	      
     /* 
@@ -5099,13 +5105,13 @@
     error= do_before_row_operations(table);
     while (error == 0 && row_start < m_rows_end) {
       char const *row_end= do_prepare_row(thd, table, row_start);
-      DBUG_ASSERT(row_end != NULL);
+      DBUG_ASSERT(row_end != NULL); // cannot happen
       DBUG_ASSERT(row_end <= m_rows_end);
 
       error= do_exec_row(table, rli);
       switch (error)
       {
-      /* Some recoverable errors */
+        /* Some recoverable errors */
       case 0:
       case HA_ERR_FOUND_DUPP_KEY:
       case HA_ERR_RECORD_CHANGED:
@@ -5222,7 +5228,7 @@
      Note that this should be the number of *bits*, not the number of
      bytes.
   */
-  byte sbuf[my_vle_sizeof(m_width)] = { 0 }; 
+  byte sbuf[my_vle_sizeof(m_width)]; 
   my_ptrdiff_t const data_size= m_rows_cur - m_rows_buf;
 
   my_size_t sbuf_len= my_vle_encode(sbuf, sizeof(sbuf), m_width);
@@ -5417,7 +5423,7 @@
 {
   DBUG_ENTER("Table_map_log_event::exec_event(st_relay_log_info*)");
   
-  THD* const thd= current_thd;
+  DBUG_ASSERT(rli->sql_thd == thd);
 
   /* Step the query id to mark what columns that are actually used. */
   pthread_mutex_lock(&LOCK_thread_count);
@@ -5491,7 +5497,8 @@
     just step the relay log position.
   */
 
-  rli->inc_event_relay_log_pos();
+  if (likely(!error))
+    rli->inc_event_relay_log_pos();
   
   DBUG_RETURN(error);
 }
@@ -5601,13 +5608,14 @@
 char const *Write_rows_log_event::do_prepare_row(THD *thd, TABLE *table,
                                                  char const *row_start)
 {
+  char const *ptr= row_start;
   DBUG_ASSERT(table != NULL);
   /*
     This assertion actually checks that there is at least as many
     columns on the slave as on the master.
   */
   DBUG_ASSERT(table->s->fields >= m_width);
-  char const *ptr= row_start;
+  DBUG_ASSERT(ptr);
   ptr= unpack_row(thd, table, table->record[0], ptr, &m_cols);
   return ptr;
 }
@@ -5827,7 +5835,7 @@
                                                   char const *row_start)
 {
   char const *ptr= row_start;
-
+  DBUG_ASSERT(ptr);
   /*
     This assertion actually checks that there is at least as many
     columns on the slave as on the master.
@@ -5877,7 +5885,6 @@
   */
   error= table->file->ha_delete_row(table->record[0]);
 
-  DBUG_PRINT("return", ("error=%d", error));
   DBUG_RETURN(error);
 }
 #endif /* !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION) */
@@ -5978,14 +5985,13 @@
                                                   char const *row_start)
 {
   char const *ptr= row_start;
-
+  DBUG_ASSERT(ptr);
   /*
     This assertion actually checks that there is at least as many
     columns on the slave as on the master.
   */
   DBUG_ASSERT(table->s->fields >= m_width);
 
-  DBUG_ASSERT(ptr != NULL);
   /* record[0] is the before image for the update */
   ptr= unpack_row(thd, table, table->record[0], ptr, &m_cols); 
   DBUG_ASSERT(ptr != NULL);
@@ -6031,7 +6037,6 @@
   */
   error= table->file->ha_update_row(table->record[0], table->record[1]);
 
-  DBUG_PRINT("return", ("error=%d", error));
   DBUG_RETURN(error);
 }
 #endif /* !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION) */

--- 1.256/sql/slave.cc	2005-08-27 10:08:15 +02:00
+++ 1.257/sql/slave.cc	2005-09-09 00:13:57 +02:00
@@ -2956,7 +2956,7 @@
     thd->lex->current_select= 0;
     if (!ev->when)
       ev->when = time(NULL);
-    ev->thd = thd;
+    ev->thd = thd; // because up to this point, ev->thd == 0
     exec_res = ev->exec_event(rli);
     DBUG_ASSERT(rli->sql_thd==thd);
     /*

--- 1.204/sql/sql_class.cc	2005-09-07 16:44:33 +02:00
+++ 1.205/sql/sql_class.cc	2005-09-09 00:13:57 +02:00
@@ -2112,21 +2112,26 @@
     /* If not, flush the event and create a new RowsEventT. */
     Rows_log_event* const 
 	ev= new RowsEventT(this, table, table_id, cols, is_transactional);
-    ev->server_id= serv_id; // I don't like this, it's too easy to forget. 
-    if (binlog_flush_and_set_pending_rows_event(ev))
-    {
-      delete ev;
+    if (unlikely(!ev))
       return NULL;
-    }
-
+    ev->server_id= serv_id; // I don't like this, it's too easy to forget. 
     /*
       We are preparing for a new kind of row-level event, so we have
       to write a map for it: unless the table was previously mapped,
       of course.
     */
-    if (!was_mapped)
-      binlog_write_table_map(table, is_transactional); // TODO catch return code
-    return ev;
+    if (unlikely(binlog_flush_and_set_pending_rows_event(ev)))
+    {
+      delete ev;
+      return NULL;
+    }
+    if (unlikely(!was_mapped &&
+                 binlog_write_table_map(table, is_transactional)))
+    {
+      /* we undo what we did: */
+      binlog_flush_and_set_pending_rows_event(NULL);
+      return NULL;
+    }
   }
   return pending;
 }

--- 1.258/sql/sql_class.h	2005-09-06 14:56:53 +02:00
+++ 1.259/sql/sql_class.h	2005-09-09 00:13:57 +02:00
@@ -53,9 +53,6 @@
 #define TC_HEURISTIC_RECOVER_ROLLBACK 2
 extern uint tc_heuristic_recover;
 
-/* Forward declaration */
-class THD;
-
 /*
   Transaction Coordinator log - a base abstract class
   for two different implementations

--- 1.2/wl1012-review-pending-comments.txt	2005-09-08 22:17:08 +02:00
+++ 1.3/wl1012-review-pending-comments.txt	2005-09-09 00:13:57 +02:00
@@ -256,9 +256,8 @@
 
 [M] Yes, I missed that one.
 
-[G2TODO]
-
-[G2TODO] scan patch for all current_thd calls.
+[G2TODO] I was able to remove 2 calls to current_thd in log_event.cc;
+[G2TODO] only the one above remains.
 
 > +    if (!use_bitbuf)
 > +      bitmap_free(&cols);
@@ -1062,8 +1061,10 @@
 
 [M] I'll do that.
 
-[G2TODO] I added detection of all alloc errors through is_valid(). More generally we need
to
-[G2TODO] detect all error return codes: scan the patch for these.
+[G2DONE] I added detection of all alloc errors through is_valid(). I
+[G2DONE] scan all function calls of the patch and added error
+[G2DONE] detection in all places except one where it's hard (and not
+[G2DONE] likely to fail).
 
 
 > +  DBUG_VOID_RETURN;
@@ -7221,10 +7222,10 @@
 > set character_set_client=latin1, collation_connection=latin1_german1_ci;
 > truncate table t1;
 > insert into t1 (b) values(@@collation_connection);
-> insert into t1 (b) values(LEAST("Müller","Muffler"));
+> insert into t1 (b) values(LEAST("Müller","Muffler"));
 > set collation_connection=latin1_german2_ci;
 > insert into t1 (b) values(@@collation_connection);
-> insert into t1 (b) values(LEAST("Müller","Muffler"));
+> insert into t1 (b) values(LEAST("Müller","Muffler"));
 > 
 > --- --master--
 > select * from t1 order by a;
@@ -7232,7 +7233,7 @@
 > 1	latin1_german1_ci
 > 2	Muffler
 > 3	latin1_german2_ci
-> 4	Müller
+> 4	Müller
 > 
 > --- --slave--
 > select * from mysqltest2.t1 order by a;
@@ -7240,8 +7241,8 @@
 > 1	latin1_german1_ci
 > 2	Muffler
 > 3	latin1_german2_ci
-> 4	Müller
-> set @a= _cp850 'Müller' collate cp850_general_ci;
+> 4	Müller
+> set @a= _cp850 'Müller' collate cp850_general_ci;
 > truncate table t1;
 > insert into t1 (b) values(collation(@a));
 > 
@@ -7286,7 +7287,7 @@
 > SET CHARACTER_SET_CLIENT=koi8r,
 > CHARACTER_SET_CONNECTION=cp1251, 
 > CHARACTER_SET_RESULTS=koi8r;
-> INSERT INTO t1 (c1, c2) VALUES ('îÕ, ÚÁ ÒÙÂÁÌËÕ','îÕ, ÚÁ ÒÙÂÁÌËÕ');
+> INSERT INTO t1 (c1, c2) VALUES ('îÕ, ÚÁ
ÒÙÂÁÌËÕ','îÕ, ÚÁ
ÒÙÂÁÌËÕ');
 > select hex(c1), hex(c2) from t1;
 > hex(c1)	hex(c2)
 > CDF32C20E7E020F0FBE1E0EBEAF3	CDF32C20E7E020F0FBE1E0EBEAF3
@@ -7953,10 +7954,10 @@
 > set character_set_client=latin1, collation_connection=latin1_german1_ci;
 > truncate table t1;
 > insert into t1 (b) values(@@collation_connection);
-> insert into t1 (b) values(LEAST("Müller","Muffler"));
+> insert into t1 (b) values(LEAST("Müller","Muffler"));
 > set collation_connection=latin1_german2_ci;
 > insert into t1 (b) values(@@collation_connection);
-> insert into t1 (b) values(LEAST("Müller","Muffler"));
+> insert into t1 (b) values(LEAST("Müller","Muffler"));
 > --disable_query_log
 > select "--- --master--" as "";
 > --enable_query_log
@@ -7980,7 +7981,7 @@
 > # which provokes error messages (like 'Illegal mix of collation') when
 > # we replay the master's INSERT/etc statements.
 > connection master;
-> set @a= _cp850 'Müller' collate cp850_general_ci;
+> set @a= _cp850 'Müller' collate cp850_general_ci;
 > truncate table t1;
 > insert into t1 (b) values(collation(@a));
 > --disable_query_log
@@ -8035,7 +8036,7 @@
 > SET CHARACTER_SET_CLIENT=koi8r,
 >  CHARACTER_SET_CONNECTION=cp1251, 
 >  CHARACTER_SET_RESULTS=koi8r;
-> INSERT INTO t1 (c1, c2) VALUES ('îÕ, ÚÁ ÒÙÂÁÌËÕ','îÕ, ÚÁ ÒÙÂÁÌËÕ');
+> INSERT INTO t1 (c1, c2) VALUES ('îÕ, ÚÁ
ÒÙÂÁÌËÕ','îÕ, ÚÁ
ÒÙÂÁÌËÕ');
 > select hex(c1), hex(c2) from t1;
 > sync_slave_with_master;
 > select hex(c1), hex(c2) from t1;

--- 1.4/sql/rpl_tblmap.cc	2005-08-05 16:23:59 +02:00
+++ 1.5/sql/rpl_tblmap.cc	2005-09-09 00:13:57 +02:00
@@ -31,6 +31,8 @@
     No "free_element" function for entries passed here, as the entries are
     allocated in a MEM_ROOT (freed as a whole in the destructor), they cannot
     be freed one by one.
+    Note that below we don't test if hash_init() succeeded. This constructor
+    is called at startup only.
   */
   (void) hash_init(&m_table_ids,&my_charset_bin,TABLE_ID_HASH_SIZE,
 		   offsetof(entry,table_id),sizeof(ulong),