Hi,
Find a preliminary review.
However, we need to discuss the changes on the sql_upate.cc and
sql_delete.cc.
Let's use skype. I think I did not understand them.
By the way, I did not checked the test cases. I am assuming that you did
exactly what
we discussed on the last time.
See some comments in-line.
Cheers
> === modified file 'sql/log_event.cc'
> --- a/sql/log_event.cc 2009-03-04 13:33:47 +0000
> +++ b/sql/log_event.cc 2009-04-07 09:55:01 +0000
> @@ -8611,7 +8611,7 @@ void Write_rows_log_event::print(FILE *f
>
> Returns TRUE if different.
> */
> -static bool record_compare(TABLE *table)
>
I don't remember how ended the discussion about the signature of
the function below. Do we have access to the readset in the object
table, dont we?
> +static bool record_compare(TABLE *table, const MY_BITMAP *cols_in_readset)
> {
> /*
> Need to set the X bit and the filler bits in both records since
> @@ -8643,14 +8643,23 @@ static bool record_compare(TABLE *table)
> }
> }
>
> - if (table->s->blob_fields + table->s->varchar_fields == 0)
> + /*
> + We can compare the record straight away if:
> + i) there are no blob and varchar fields
> + ii) all columns were read or the slave SE provides partial reads
> + */
> + if ((table->s->blob_fields + table->s->varchar_fields == 0)
> &&
> + (bitmap_is_set_all(cols_in_readset) ||
> + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)))
> {
> result= cmp_record(table,record[1]);
> goto record_compare_exit;
> }
>
> - /* Compare null bits */
> - if (memcmp(table->null_flags,
> + /* Compare null bits only if all fields were read or slave SE has partial reads
> */
> + if ((bitmap_is_set_all(cols_in_readset) ||
> + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> &&
> + memcmp(table->null_flags,
> table->null_flags+table->s->rec_buff_length,
> table->s->null_bytes))
> {
> @@ -8661,10 +8670,14 @@ static bool record_compare(TABLE *table)
> /* Compare updated fields */
> for (Field **ptr=table->field ; *ptr ; ptr++)
> {
> - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> - {
> - result= TRUE;
> - goto record_compare_exit;
> + /* compare field if it is set */
> + if (bitmap_is_set(cols_in_readset, (*ptr)->field_index))
> + {
> + if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> + {
> + result= TRUE;
> + goto record_compare_exit;
> + }
> }
> }
>
> @@ -8875,7 +8888,7 @@ int Rows_log_event::find_row(const Relay
> */
> DBUG_PRINT("info",("non-unique index, scanning it to find matching record"));
>
> - while (record_compare(table))
> + while (record_compare(table, &m_cols))
> {
> /*
> We need to set the null bytes to ensure that the filler bit
> @@ -8956,7 +8969,7 @@ int Rows_log_event::find_row(const Relay
> goto err;
> }
> }
> - while (restart_count < 2 && record_compare(table));
> + while (restart_count < 2 && record_compare(table, &m_cols));
>
> /*
> Note: above record_compare will take into accout all record fields
>
> === modified file 'sql/log_event_old.cc'
> --- a/sql/log_event_old.cc 2008-12-10 14:30:52 +0000
> +++ b/sql/log_event_old.cc 2009-04-07 09:55:01 +0000
> @@ -313,7 +313,7 @@ last_uniq_key(TABLE *table, uint keyno)
>
> Returns TRUE if different.
> */
> -static bool record_compare(TABLE *table)
> +static bool record_compare(TABLE *table, const MY_BITMAP *cols_in_readset)
> {
> /*
> Need to set the X bit and the filler bits in both records since
> @@ -342,16 +342,25 @@ static bool record_compare(TABLE *table)
> }
> }
>
> - if (table->s->blob_fields + table->s->varchar_fields == 0)
> + /*
> + We can compare the record straight away if:
> + i) there are no blob and varchar fields
> + ii) all columns were read or the slave SE provides partial reads
> + */
> + if ((table->s->blob_fields + table->s->varchar_fields == 0)
> &&
> + (bitmap_is_set_all(cols_in_readset) ||
> + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)))
> {
> result= cmp_record(table,record[1]);
> goto record_compare_exit;
> }
>
> - /* Compare null bits */
> - if (memcmp(table->null_flags,
> - table->null_flags+table->s->rec_buff_length,
> - table->s->null_bytes))
> + /* Compare null bits only if all fields were read or slave SE has partial reads
> */
> + if ((bitmap_is_set_all(cols_in_readset) ||
> + (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> &&
> + memcmp(table->null_flags,
> + table->null_flags+table->s->rec_buff_length,
> + table->s->null_bytes))
> {
> result= TRUE; // Diff in NULL value
> goto record_compare_exit;
> @@ -360,10 +369,14 @@ static bool record_compare(TABLE *table)
> /* Compare updated fields */
> for (Field **ptr=table->field ; *ptr ; ptr++)
> {
> - if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> - {
> - result= TRUE;
> - goto record_compare_exit;
> + /* compare field if it is set */
> + if (bitmap_is_set(cols_in_readset, (*ptr)->field_index))
> + {
> + if ((*ptr)->cmp_binary_offset(table->s->rec_buff_length))
> + {
> + result= TRUE;
> + goto record_compare_exit;
> + }
> }
> }
>
> @@ -644,6 +657,7 @@ replace_record(THD *thd, TABLE *table,
>
> @param table Pointer to table to search
> @param key Pointer to key to use for search, if table has key
> + @param cols Bitmap denoting the columns read in the BI image.
>
> @pre <code>table->record[0]</code> shall contain the row to locate
> and <code>key</code> shall contain a key to use for searching, if
> @@ -659,7 +673,7 @@ replace_record(THD *thd, TABLE *table,
> <code>table->record[1]</code>, error code otherwise.
> */
>
> -static int find_and_fetch_row(TABLE *table, uchar *key)
> +static int find_and_fetch_row(TABLE *table, uchar *key, MY_BITMAP *cols)
> {
> DBUG_ENTER("find_and_fetch_row(TABLE *table, uchar *key, uchar *record)");
> DBUG_PRINT("enter", ("table: %p, key: %p record: %p",
> @@ -761,7 +775,7 @@ static int find_and_fetch_row(TABLE *tab
> DBUG_RETURN(0);
> }
>
> - while (record_compare(table))
> + while (record_compare(table, cols))
> {
> int error;
>
> @@ -837,7 +851,7 @@ static int find_and_fetch_row(TABLE *tab
> DBUG_RETURN(error);
> }
> }
> - while (restart_count < 2 && record_compare(table));
> + while (restart_count < 2 && record_compare(table, cols));
>
> /*
> Have to restart the scan to be able to fetch the next row.
> @@ -1060,7 +1074,7 @@ int Delete_rows_log_event_old::do_exec_r
> int error;
> DBUG_ASSERT(table != NULL);
>
> - if (!(error= ::find_and_fetch_row(table, m_key)))
> + if (!(error= ::find_and_fetch_row(table, m_key, &m_cols)))
> {
> /*
> Now we should have the right row to delete. We are using
> @@ -1168,7 +1182,7 @@ int Update_rows_log_event_old::do_exec_r
> {
> DBUG_ASSERT(table != NULL);
>
> - int error= ::find_and_fetch_row(table, m_key);
> + int error= ::find_and_fetch_row(table, m_key, &m_cols);
> if (error)
> return error;
>
> @@ -2387,7 +2401,7 @@ int Old_rows_log_event::find_row(const R
> */
> DBUG_PRINT("info",("non-unique index, scanning it to find matching record"));
>
> - while (record_compare(table))
> + while (record_compare(table, &m_cols))
> {
> /*
> We need to set the null bytes to ensure that the filler bit
> @@ -2463,7 +2477,7 @@ int Old_rows_log_event::find_row(const R
> DBUG_RETURN(error);
> }
> }
> - while (restart_count < 2 && record_compare(table));
> + while (restart_count < 2 && record_compare(table, &m_cols));
>
> /*
> Note: above record_compare will take into accout all record fields
>
> === modified file 'sql/sql_delete.cc'
> --- a/sql/sql_delete.cc 2009-03-04 13:31:31 +0000
> +++ b/sql/sql_delete.cc 2009-04-07 09:55:01 +0000
> @@ -292,7 +292,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *
>
>
> table->mark_columns_needed_for_delete();
> -
> + /* conditionally mark columns needed for BI in RBR */
> + table->mark_columns_needed_for_rbr_bi();
> while (!(error=info.read_record(&info)) && !thd->killed &&
> ! thd->is_error())
> {
> @@ -342,6 +343,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *
> else
> table->file->unlock_row(); // Row failed selection, release lock on it
> }
> + /* conditionally restore read_set bitmap after read */
> + table->restore_columns_maps_after_rbr_bi();
> killed_status= thd->killed;
> if (killed_status != THD::NOT_KILLED || thd->is_error())
> error= 1; // Aborted
>
> === modified file 'sql/sql_update.cc'
> --- a/sql/sql_update.cc 2009-02-16 21:18:45 +0000
> +++ b/sql/sql_update.cc 2009-04-07 09:55:01 +0000
> @@ -475,6 +475,8 @@ int mysql_update(THD *thd,
> thd_proc_info(thd, "Searching rows for update");
> ha_rows tmp_limit= limit;
>
> + /* conditionally mark columns needed for BI in RBR */
> + table->mark_columns_needed_for_rbr_bi();
> while (!(error=info.read_record(&info)) && !thd->killed)
> {
> if (!(select && select->skip_record()))
> @@ -498,6 +500,8 @@ int mysql_update(THD *thd,
> else
> table->file->unlock_row();
> }
> + /* conditionally restore read_set bitmap after read */
> + table->restore_columns_maps_after_rbr_bi();
> if (thd->killed && !error)
> error= 1; // Aborted
> limit= tmp_limit;
> @@ -572,6 +576,9 @@ int mysql_update(THD *thd,
> if (table->file->ha_table_flags() & HA_PARTIAL_COLUMN_READ)
> table->prepare_for_position();
>
> + /* conditionally mark columns needed for BI in RBR */
> + table->mark_columns_needed_for_rbr_bi();
> +
> /*
> We can use compare_record() to optimize away updates if
> the table handler is returning all columns OR if
> @@ -775,6 +782,8 @@ int mysql_update(THD *thd,
> updated-= dup_key_found;
> if (will_batch)
> table->file->end_bulk_update();
> + /* conditionally restore read_set bitmap after read */
> + table->restore_columns_maps_after_rbr_bi();
> table->file->try_semi_consistent_read(0);
>
> if (!transactional_table && updated > 0)
>
> === modified file 'sql/table.cc'
> --- a/sql/table.cc 2009-03-04 13:33:47 +0000
> +++ b/sql/table.cc 2009-04-07 09:55:01 +0000
> @@ -4679,6 +4679,72 @@ void TABLE::mark_columns_needed_for_inse
> mark_auto_increment_column();
> }
>
>
Do you always use the TABLE::mark_columns_needed_for_rbr_bi() and
void TABLE::restore_columns_maps_after_rbr_bi() together? If so, please
move the initialization and destruction of the
structure rbr_bi_tmp_map.bitmap to the TABLE:TABLE and TABLE:~TABLE,
respectively.
> +void TABLE::mark_columns_needed_for_rbr_bi()
> +{
> + /* sets read set needed for RBR */
> + if ((!no_replicate &&
> + in_use->current_stmt_binlog_row_based &&
> + mysql_bin_log.is_open()))
> + {
> +
> + /* save a copy of the old read_set */
> + if (!rbr_bi_tmp_map.bitmap)
> + bitmap_init(&rbr_bi_tmp_map, NULL, read_set->n_bits, FALSE);
> +
> + bitmap_copy(&rbr_bi_tmp_map, read_set);
> +
> + /*
> + use all columns if there is no primary key
> + or engine does not provide partial fetch.
> + */
>
And about unique indexes?
Why aren't you considering this?
If there is no technical issue write a comment on that before the if
and file a bug report in order that we don't forget it.
You don't need to implement this as part of this bug.
> + if (s->primary_key == MAX_KEY ||
> + !(file->ha_table_flags() & HA_PARTIAL_COLUMN_READ))
> + /*
> + We could use the s->all_set bitmap here, but since we
> + need to handle the restore when setting all columns
> + for the primary key (the else branch below), we will
> + follow the same pattern with the use_all_columns in
> + read_set.
> + */
> + bitmap_set_all(read_set);
> + else
> + {
> + /* mark the primary key */
>
Can you explain why you are preserving the bits in the original readset?
-----------
> + mark_columns_used_by_index_no_reset(s->primary_key, read_set);
>
----------
> + file->column_bitmaps_signal();
> + }
> + }
> +}
> +
> +void TABLE::restore_columns_maps_after_rbr_bi()
> +{
> + if ((!no_replicate &&
> + in_use->current_stmt_binlog_row_based &&
> + mysql_bin_log.is_open()))
> + {
> + /*
> + If the bitmap exists that means that the proper
> + mark method was called before this one. Otherwise
> + just ignore.
> + */
> + if (rbr_bi_tmp_map.bitmap != NULL)
> + {
> + /*
> + Restore old read set by intersecting both: the bitmap set after marking
> + and the saved bitmap. This handles the following cases:
> + a) mask out the original readset when marked all columns
> + b) mask out primary key if it was not set in the first place
> + Note that partial primary key columns might have been set
> + even before "mark_columns_needed_for_rbr_bi" and we want
> + to preserve those as well.
> + */
>
Why don't you simply copy the saved readset back?
Is the saved_readset & changed_readet == saved_readset?
Can you explain when this is different?
See comment above.
----------
> + bitmap_intersect(read_set, &rbr_bi_tmp_map);
>
----------
> +
> + /* free memory used by temporary bitmap */
> + bitmap_free(&rbr_bi_tmp_map);
>
See comment above.
> + }
> + }
> +}
>
> /*
> Cleanup this table for re-execution.
>
> === modified file 'sql/table.h'
> --- a/sql/table.h 2009-03-04 20:29:16 +0000
> +++ b/sql/table.h 2009-04-07 09:55:01 +0000
> @@ -678,7 +678,7 @@ public:
> const char *alias; /* alias or table name */
> uchar *null_flags;
> my_bitmap_map *bitmap_init_value;
> - MY_BITMAP def_read_set, def_write_set, tmp_set; /* containers */
> + MY_BITMAP def_read_set, def_write_set, tmp_set, rbr_bi_tmp_map; /* containers
> */
> MY_BITMAP *read_set, *write_set; /* Active column sets */
> /*
> The ID of the query that opened and is using this table. Has different
> @@ -804,6 +804,8 @@ public:
> void mark_columns_needed_for_update(void);
> void mark_columns_needed_for_delete(void);
> void mark_columns_needed_for_insert(void);
> + void mark_columns_needed_for_rbr_bi(void);
> + void restore_columns_maps_after_rbr_bi(void);
> inline void column_bitmaps_set(MY_BITMAP *read_set_arg,
> MY_BITMAP *write_set_arg)
> {
>
> ------------------------------------------------------------------------
>
> This body part will be downloaded on demand.
Cheers.
