Cache check_table_binlog_row_based and mark_trx_read_write

Benefits:
- Speeds up insert,write and delete by avoiding 1-2 function calls per write/update/delete.
- Avoiding calling write_locked_table_maps() if not needed.
- The inlined code is much smaller than before
- Updating of table->s->cached_row_logging_check moved to when table is opened
- Moved some bool values together in handler class to get better alignment.

sql/handler.cc

@@ -34,7 +34,6 @@
 #include "discover.h"           // extension_based_table_discovery, etc
 #include "log_event.h"          // *_rows_log_event
 #include "create_options.h"
-#include "rpl_filter.h"
 #include <myisampack.h>
 #include "transaction.h"
 #include "myisam.h"
@@ -3931,9 +3930,7 @@ int handler::ha_check(THD *thd, HA_CHECK_OPT *check_opt)
   if it is started.
 */
 
-inline
+void handler::mark_trx_read_write_internal()
-void
-handler::mark_trx_read_write()
 {
   Ha_trx_info *ha_info= &ha_thd()->ha_data[ht->slot].ha_info[0];
   /*
@@ -5579,30 +5576,45 @@ bool ha_show_status(THD *thd, handlerton *db_type, enum ha_stat_type stat)
   correct for the table.
 
   A row in the given table should be replicated if:
+  - It's not called by partition engine
   - Row-based replication is enabled in the current thread
   - The binlog is enabled
   - It is not a temporary table
   - The binary log is open
   - The database the table resides in shall be binlogged (binlog_*_db rules)
   - table is not mysql.event
+
+  RETURN VALUE
+    0  No binary logging in row format
+    1  Row needs to be logged
 */
 
-static bool check_table_binlog_row_based(THD *thd, TABLE *table)
+inline bool handler::check_table_binlog_row_based(bool binlog_row)
 {
-  if (table->s->cached_row_logging_check == -1)
+  if (unlikely((table->in_use->variables.sql_log_bin_off)))
+    return 0;                            /* Called by partitioning engine */
+  if (unlikely((!check_table_binlog_row_based_done)))
   {
-    int const check(table->s->tmp_table == NO_TMP_TABLE &&
+    check_table_binlog_row_based_done= 1;
-                    ! table->no_replicate &&
+    check_table_binlog_row_based_result=
-                    binlog_filter->db_ok(table->s->db.str));
+      check_table_binlog_row_based_internal(binlog_row);
-    table->s->cached_row_logging_check= check;
   }
+  return check_table_binlog_row_based_result;
+}
 
-  DBUG_ASSERT(table->s->cached_row_logging_check == 0 ||
+bool handler::check_table_binlog_row_based_internal(bool binlog_row)
-              table->s->cached_row_logging_check == 1);
+{
+  THD *thd;
 
-  return (thd->is_current_stmt_binlog_format_row() &&
+  /* only InnoDB tables will be replicated through binlog emulation */
-          table->s->cached_row_logging_check &&
+  if (binlog_row &&
-#ifdef WITH_WSREP
+      WSREP_EMULATE_BINLOG(thd) &&
+      table->file->partition_ht()->db_type != DB_TYPE_INNODB)
+    return 0;
+
+  thd= table->in_use;
+  return (table->s->cached_row_logging_check &&
+          thd->is_current_stmt_binlog_format_row() &&
           /*
             Wsrep partially enables binary logging if it have not been
             explicitly turned on. As a result we return 'true' if we are in
@@ -5617,14 +5629,13 @@ static bool check_table_binlog_row_based(THD *thd, TABLE *table)
 
             Otherwise, return 'true' if binary logging is on.
           */
-          (thd->variables.sql_log_bin_off != 1) &&
+          IF_WSREP(((WSREP_EMULATE_BINLOG(thd) &&
-          ((WSREP_EMULATE_BINLOG(thd) && (thd->wsrep_exec_mode != REPL_RECV)) ||
+                     (thd->wsrep_exec_mode != REPL_RECV)) ||
-           ((WSREP(thd) || (thd->variables.option_bits & OPTION_BIN_LOG)) &&
+                    ((WSREP(thd) ||
-            mysql_bin_log.is_open())));
+                      (thd->variables.option_bits & OPTION_BIN_LOG)) &&
-#else
+                     mysql_bin_log.is_open())),
-          (thd->variables.option_bits & OPTION_BIN_LOG) &&
+                   (thd->variables.option_bits & OPTION_BIN_LOG) &&
-          mysql_bin_log.is_open());
+                   mysql_bin_log.is_open()));
-#endif
 }
 
 
@@ -5658,54 +5669,51 @@ static int write_locked_table_maps(THD *thd)
 
   DBUG_PRINT("debug", ("get_binlog_table_maps(): %d", thd->get_binlog_table_maps()));
 
-  if (thd->get_binlog_table_maps() == 0)
+  MYSQL_LOCK *locks[2];
+  locks[0]= thd->extra_lock;
+  locks[1]= thd->lock;
+  my_bool with_annotate= thd->variables.binlog_annotate_row_events &&
+    thd->query() && thd->query_length();
+
+  for (uint i= 0 ; i < sizeof(locks)/sizeof(*locks) ; ++i )
   {
-    MYSQL_LOCK *locks[2];
+    MYSQL_LOCK const *const lock= locks[i];
-    locks[0]= thd->extra_lock;
+    if (lock == NULL)
-    locks[1]= thd->lock;
+      continue;
-    my_bool with_annotate= thd->variables.binlog_annotate_row_events &&
-                           thd->query() && thd->query_length();
 
-    for (uint i= 0 ; i < sizeof(locks)/sizeof(*locks) ; ++i )
+    TABLE **const end_ptr= lock->table + lock->table_count;
+    for (TABLE **table_ptr= lock->table ; 
+         table_ptr != end_ptr ;
+         ++table_ptr)
     {
-      MYSQL_LOCK const *const lock= locks[i];
+      TABLE *const table= *table_ptr;
-      if (lock == NULL)
+      DBUG_PRINT("info", ("Checking table %s", table->s->table_name.str));
-        continue;
+      if (table->current_lock == F_WRLCK &&
-
+          table->file->check_table_binlog_row_based(0))
-      TABLE **const end_ptr= lock->table + lock->table_count;
-      for (TABLE **table_ptr= lock->table ; 
-           table_ptr != end_ptr ;
-           ++table_ptr)
       {
-        TABLE *const table= *table_ptr;
+        /*
-        DBUG_PRINT("info", ("Checking table %s", table->s->table_name.str));
+          We need to have a transactional behavior for SQLCOM_CREATE_TABLE
-        if (table->current_lock == F_WRLCK &&
+          (e.g. CREATE TABLE... SELECT * FROM TABLE) in order to keep a
-            check_table_binlog_row_based(thd, table))
+          compatible behavior with the STMT based replication even when
-        {
+          the table is not transactional. In other words, if the operation
-          /*
+          fails while executing the insert phase nothing is written to the
-            We need to have a transactional behavior for SQLCOM_CREATE_TABLE
+          binlog.
-            (e.g. CREATE TABLE... SELECT * FROM TABLE) in order to keep a
-            compatible behavior with the STMT based replication even when
-            the table is not transactional. In other words, if the operation
-            fails while executing the insert phase nothing is written to the
-            binlog.
 
-            Note that at this point, we check the type of a set of tables to
+          Note that at this point, we check the type of a set of tables to
-            create the table map events. In the function binlog_log_row(),
+          create the table map events. In the function binlog_log_row(),
-            which calls the current function, we check the type of the table
+          which calls the current function, we check the type of the table
-            of the current row.
+          of the current row.
-          */
+        */
-          bool const has_trans= thd->lex->sql_command == SQLCOM_CREATE_TABLE ||
+        bool const has_trans= thd->lex->sql_command == SQLCOM_CREATE_TABLE ||
-                                table->file->has_transactions();
+          table->file->has_transactions();
-          int const error= thd->binlog_write_table_map(table, has_trans,
+        int const error= thd->binlog_write_table_map(table, has_trans,
-                                                       &with_annotate);
+                                                     &with_annotate);
-          /*
+        /*
-            If an error occurs, it is the responsibility of the caller to
+          If an error occurs, it is the responsibility of the caller to
-            roll back the transaction.
+          roll back the transaction.
-          */
+        */
-          if (unlikely(error))
+        if (unlikely(error))
-            DBUG_RETURN(1);
+          DBUG_RETURN(1);
-        }
       }
     }
   }
@@ -5715,44 +5723,50 @@ static int write_locked_table_maps(THD *thd)
 
 typedef bool Log_func(THD*, TABLE*, bool, const uchar*, const uchar*);
 
-static int binlog_log_row(TABLE* table,
+
-                          const uchar *before_record,
+
-                          const uchar *after_record,
+static int binlog_log_row_internal(TABLE* table,
-                          Log_func *log_func)
+                                   const uchar *before_record,
+                                   const uchar *after_record,
+                                   Log_func *log_func)
 {
   bool error= 0;
   THD *const thd= table->in_use;
 
-  /* only InnoDB tables will be replicated through binlog emulation */
+  /*
-  if (WSREP_EMULATE_BINLOG(thd) &&
+    If there are no table maps written to the binary log, this is
-      table->file->partition_ht()->db_type != DB_TYPE_INNODB)
+    the first row handled in this statement. In that case, we need
-    return 0;
+    to write table maps for all locked tables to the binary log.
-
+  */
-  if (check_table_binlog_row_based(thd, table))
+  if (likely(!(error= ((thd->get_binlog_table_maps() == 0 &&
+                        write_locked_table_maps(thd))))))
   {
     /*
-      If there are no table maps written to the binary log, this is
+      We need to have a transactional behavior for SQLCOM_CREATE_TABLE
-      the first row handled in this statement. In that case, we need
+      (i.e. CREATE TABLE... SELECT * FROM TABLE) in order to keep a
-      to write table maps for all locked tables to the binary log.
+      compatible behavior with the STMT based replication even when
+      the table is not transactional. In other words, if the operation
+      fails while executing the insert phase nothing is written to the
+      binlog.
     */
-    if (likely(!(error= write_locked_table_maps(thd))))
+    bool const has_trans= thd->lex->sql_command == SQLCOM_CREATE_TABLE ||
-    {
+      table->file->has_transactions();
-      /*
+    error= (*log_func)(thd, table, has_trans, before_record, after_record);
-         We need to have a transactional behavior for SQLCOM_CREATE_TABLE
-         (i.e. CREATE TABLE... SELECT * FROM TABLE) in order to keep a
-         compatible behavior with the STMT based replication even when
-         the table is not transactional. In other words, if the operation
-         fails while executing the insert phase nothing is written to the
-         binlog.
-         */
-      bool const has_trans= thd->lex->sql_command == SQLCOM_CREATE_TABLE ||
-                            table->file->has_transactions();
-      error= (*log_func)(thd, table, has_trans, before_record, after_record);
-    }
   }
   return error ? HA_ERR_RBR_LOGGING_FAILED : 0;
 }
 
+static inline int binlog_log_row(TABLE* table,
+                                 const uchar *before_record,
+                                 const uchar *after_record,
+                                 Log_func *log_func)
+{
+  if (!table->file->check_table_binlog_row_based(1))
+    return 0;
+  return binlog_log_row_internal(table, before_record, after_record, log_func);
+}
+
+
 int handler::ha_external_lock(THD *thd, int lock_type)
 {
   int error;
@@ -5851,6 +5865,8 @@ int handler::ha_reset()
   table->default_column_bitmaps();
   pushed_cond= NULL;
   tracker= NULL;
+  mark_trx_read_write_done= check_table_binlog_row_based_done=
+    check_table_binlog_row_based_result= 0;
   /* Reset information about pushed engine conditions */
   cancel_pushed_idx_cond();
   /* Reset information about pushed index conditions */
@@ -5875,14 +5891,13 @@ int handler::ha_write_row(uchar *buf)
                       { error= write_row(buf); })
 
   MYSQL_INSERT_ROW_DONE(error);
-  if (unlikely(error))
+  if (likely(!error))
-    DBUG_RETURN(error);
+  {
-  rows_changed++;
+    rows_changed++;
-  if (unlikely(error= binlog_log_row(table, 0, buf, log_func)))
+    error= binlog_log_row(table, 0, buf, log_func);
-    DBUG_RETURN(error); /* purecov: inspected */
+  }
-
   DEBUG_SYNC_C("ha_write_row_end");
-  DBUG_RETURN(0);
+  DBUG_RETURN(error);
 }
 
 
@@ -5908,12 +5923,12 @@ int handler::ha_update_row(const uchar *old_data, uchar *new_data)
                       { error= update_row(old_data, new_data);})
 
   MYSQL_UPDATE_ROW_DONE(error);
-  if (unlikely(error))
+  if (likely(!error))
-    return error;
+  {
-  rows_changed++;
+    rows_changed++;
-  if (unlikely(error= binlog_log_row(table, old_data, new_data, log_func)))
+    error= binlog_log_row(table, old_data, new_data, log_func);
-    return error;
+  }
-  return 0;
+  return error;
 }
 
 int handler::ha_delete_row(const uchar *buf)
@@ -5935,12 +5950,12 @@ int handler::ha_delete_row(const uchar *buf)
   TABLE_IO_WAIT(tracker, m_psi, PSI_TABLE_DELETE_ROW, active_index, 0,
     { error= delete_row(buf);})
   MYSQL_DELETE_ROW_DONE(error);
-  if (unlikely(error))
+  if (likely(!error))
-    return error;
+  {
-  rows_changed++;
+    rows_changed++;
-  if (unlikely(error= binlog_log_row(table, buf, 0, log_func)))
+    error= binlog_log_row(table, buf, 0, log_func);
-    return error;
+  }
-  return 0;
+  return error;
 }
 
 

sql/handler.h

@@ -2581,11 +2581,6 @@ public:
   RANGE_SEQ_IF mrr_funcs;  /* Range sequence traversal functions */
   HANDLER_BUFFER *multi_range_buffer; /* MRR buffer info */
   uint ranges_in_seq; /* Total number of ranges in the traversed sequence */
-  /* TRUE <=> source MRR ranges and the output are ordered */
-  bool mrr_is_output_sorted;
-
-  /** TRUE <=> we're currently traversing a range in mrr_cur_range. */
-  bool mrr_have_range;
   /** Current range (the one we're now returning rows from) */
   KEY_MULTI_RANGE mrr_cur_range;
 
@@ -2593,23 +2588,32 @@ public:
   key_range save_end_range, *end_range;
   KEY_PART_INFO *range_key_part;
   int key_compare_result_on_equal;
-  bool eq_range;
-  bool internal_tmp_table;                      /* If internal tmp table */
 
-  uint errkey;				/* Last dup key */
+  /* TRUE <=> source MRR ranges and the output are ordered */
-  uint key_used_on_scan;
+  bool mrr_is_output_sorted;
-  uint active_index;
+  /** TRUE <=> we're currently traversing a range in mrr_cur_range. */
+  bool mrr_have_range;
+  bool eq_range;
+  bool internal_tmp_table;                 /* If internal tmp table */
+  bool implicit_emptied;                   /* Can be !=0 only if HEAP */
+  bool mark_trx_read_write_done;           /* mark_trx_read_write was called */
+  bool check_table_binlog_row_based_done; /* check_table_binlog.. was called */
+  bool check_table_binlog_row_based_result; /* cached check_table_binlog... */
   /* 
     TRUE <=> the engine guarantees that returned records are within the range
     being scanned.
   */
   bool in_range_check_pushed_down;
 
+  uint errkey;                             /* Last dup key */
+  uint key_used_on_scan;
+  uint active_index;
+
   /** Length of ref (1-8 or the clustered key length) */
   uint ref_length;
   FT_INFO *ft_handler;
   enum {NONE=0, INDEX, RND} inited;
-  bool implicit_emptied;                /* Can be !=0 only if HEAP */
+
   const COND *pushed_cond;
   /**
     next_insert_id is the next value which should be inserted into the
@@ -2693,11 +2697,16 @@ public:
   handler(handlerton *ht_arg, TABLE_SHARE *share_arg)
     :table_share(share_arg), table(0),
     estimation_rows_to_insert(0), ht(ht_arg),
-    ref(0), end_range(NULL), key_used_on_scan(MAX_KEY), active_index(MAX_KEY),
+    ref(0), end_range(NULL),
+    implicit_emptied(0),
+    mark_trx_read_write_done(0),
+    check_table_binlog_row_based_done(0),
+    check_table_binlog_row_based_result(0),
     in_range_check_pushed_down(FALSE),
+    key_used_on_scan(MAX_KEY),
+    active_index(MAX_KEY),
     ref_length(sizeof(my_off_t)),
     ft_handler(0), inited(NONE),
-    implicit_emptied(0),
     pushed_cond(0), next_insert_id(0), insert_id_for_cur_row(0),
     tracker(NULL),
     pushed_idx_cond(NULL),
@@ -3875,10 +3884,22 @@ protected:
   */
   virtual int delete_table(const char *name);
 
+public:
+  inline bool check_table_binlog_row_based(bool binlog_row);
 private:
+  /* Cache result to avoid extra calls */
+  inline void mark_trx_read_write()
+  {
+    if (unlikely(!mark_trx_read_write_done))
+    {
+      mark_trx_read_write_done= 1;
+      mark_trx_read_write_internal();
+    }
+  }
+  void mark_trx_read_write_internal();
+  bool check_table_binlog_row_based_internal(bool binlog_row);
+
   /* Private helpers */
-  inline void mark_trx_read_write();
-private:
   inline void increment_statistics(ulong SSV::*offset) const;
   inline void decrement_statistics(ulong SSV::*offset) const;
 

sql/table.cc

@@ -40,6 +40,7 @@
 #include "discover.h"
 #include "mdl.h"                 // MDL_wait_for_graph_visitor
 #include "sql_view.h"
+#include "rpl_filter.h"
 
 /* INFORMATION_SCHEMA name */
 LEX_STRING INFORMATION_SCHEMA_NAME= {C_STRING_WITH_LEN("information_schema")};
@@ -316,7 +317,8 @@ TABLE_SHARE *alloc_table_share(const char *db, const char *table_name,
     share->normalized_path.length= path_length;
     share->table_category= get_table_category(& share->db, & share->table_name);
     share->open_errno= ENOENT;
-    share->cached_row_logging_check= -1;
+    /* The following will be fixed in open_table_from_share */
+    share->cached_row_logging_check= 1;
 
     init_sql_alloc(&share->stats_cb.mem_root, TABLE_ALLOC_BLOCK_SIZE, 0, MYF(0));
 
@@ -381,7 +383,7 @@ void init_tmp_table_share(THD *thd, TABLE_SHARE *share, const char *key,
   share->path.length= share->normalized_path.length= strlen(path);
   share->frm_version= 		 FRM_VER_TRUE_VARCHAR;
 
-  share->cached_row_logging_check= -1;
+  share->cached_row_logging_check= 0;           // No row logging
 
   /*
     table_map_id is also used for MERGE tables to suppress repeated
@@ -2974,6 +2976,9 @@ partititon_err:
     outparam->no_replicate= FALSE;
   }
 
+  if (outparam->no_replicate || !binlog_filter->db_ok(outparam->s->db.str))
+    outparam->s->cached_row_logging_check= 0;   // No row based replication
+
   /* Increment the opened_tables counter, only when open flags set. */
   if (db_stat)
     thd->status_var.opened_tables++;