Hi!
I have review the diffs between:
c0bd9cdf131412fe26fa96d65e896c86208a3c8b (12.1 tree)
d3ed67a9bd5a880872b35c22345c3413c49ba21a 10.6 tree
I also did a diff between rpl_record.cc for the two version and did a
deep review of all the changes for that file. This explains why there
are two different iterations over
rpl_record.cc.
------------
rpl_records.cc
+ /*
+ TODO BN: I Think null_ptr needs to be initially offset like in the base
+ patch
*/
null_value= rpl_bitmap_is_set(null_ptr, null_pos++);
Please clarify what you mean with this.
The original code did:
uchar const *null_ptr= row_data;
unsigned int null_mask= 1U;
// The "current" null bits
unsigned int null_bits= *null_ptr++;
and then for each field:
if ((null_mask & 0xFF) == 0)
{
DBUG_ASSERT(null_ptr < row_data + master_null_byte_count);
null_mask= 1U;
null_bits= *null_ptr++;
}
ending with
null_mask <<= 1;
My code should be identical to this one, but less error prone as
everything is done in one line.
@@ -982,7 +982,6 @@ table_def::compatible_with(THD *thd, rpl_group_info *rgi,
}
if (convtype == CONV_TYPE_PRECISE && tmp_table != NULL)
tmp_table->field[conv_table_idx]= NULL;
- conv_table_idx++;
}
Good catch. I wonder why the tests didn't catch this.
I assume your extended test did find this. Is that the case?
RPL_TABLE_LIST::check_wrong_column_usage()
- if (give_compatibility_error(rgi, col))
- DBUG_RETURN(1);
+ has_err= give_compatibility_error(rgi, col) || has_err;
Please add a note to the function description that we give errors for
all wrong colums, not just 'Give an error'
Note that the common way to do this is:
has_err|= give_compatibility_error(rgi, col);
(Does not need to be changed)
Move DBUG_ASSERT directly after bitmap_is_set().
The reason is that the assert will trigger only if the bitmap is wrong,
not depending on any interleaving code.
It is best to have DBUG_ASSERT() close to the code/construct we are checking.
@@ -1056,27 +1057,31 @@ bool
RPL_TABLE_LIST::give_compatibility_error(rpl_group_info *rgi, uint
col)
switch (m_tabledef.master_to_slave_error[col]) {
case SLAVE_FIELD_NAME_MISSING:
+ DBUG_ASSERT(m_tabledef.master_column_name[col]);
+ DBUG_ASSERT(m_tabledef.master_to_slave_map[col] == UINT_MAX32);
I don't think we should check
DBUG_ASSERT(m_tabledef.master_to_slave_map[col] == UINT_MAX32);
As the setting the value to UINT_MAX32 is just a precausing and no
code is depending on that. The setting of the value can go away
without any side effect.
We also say in other code comments:
"Note master_to_slave_map[col] is set to UINT_MAX32, but is never actually used"
Now you are using it in asserts, which contradicts the other comments.
After all, you are not testing in other places that master_to_slave_map[]
is not UINT_MAX32.
@@ -1056,27 +1057,31 @@ bool
RPL_TABLE_LIST::give_compatibility_error(rpl_group_info *rgi, uint
col)
switch (m_tabledef.master_to_slave_error[col]) {
...
+ my_free(m_tabledef.master_column_name[col]);
+ m_tabledef.master_column_name[col]= NULL;
break;
Why freeing the table name?
This deserves a comment!
If we would call give_compatibility_error() twice with same column,
this would cause an assert.
It would be better to free all master_column_names at the end of the
event instead of one at a time.
This is needed as if things break because of some other error
we will never free the names.
if (!(slave_type_conversions_options &
- SLAVE_TYPE_CONVERSIONS_ERROR_IF_MISSING_FIELD))
+ (1ULL << SLAVE_TYPE_CONVERSIONS_ERROR_IF_MISSING_FIELD)))
Sorry, I missed that. In most part of the code, the bitmaps are direct
pointers. It some point we should fix SLAVE_TYPE_CONVERSIONS to be the same.
In the new code we have:
std::string master_col_name_cppstr= opt_metadata.m_column_name[col];
Please do not use std::string for anything!
We have already own string functions that are properly monitored
and we should use these everywhere!
At some point, PLEASE remove all std:: from struct Optional_metadata_fields
We should also allocate all memory in mem_root that is tied to the main
event. This way all names will kept until end of the main event and
we do not have to allocate new ones in RPL_TABLE_LIST::create_column_mapping
The main change I would like to see in the patch is to either get rid of
the allocation of column names on the stack (in which case we do not need
to do any allocations in create_column_mapping() or
alternatively change so that we free all master_column_names in one
place.
---------------------------------------
Review of c0bd9cdf131412fe26fa96d65e896c86208a3c8b (merge of things to 12.1):
...
Note for Review:
- MDEV-36892 is not addressed, so the clause and associated code from
the 10.6 patch is removed:
"Data Loss Replicating Persistent Fields if Slave Has Different Function"
"""
- Store a table's original write_set in cond_set, so we can later
cross-reference it when automatically populating fields (i.e. so we
know not to override a replicated value).
"""
Noted.
diff --git a/mysql-test/suite/rpl/r/rpl_extra_col_slave_innodb.result
b/mysql-test/suite/rpl/r/rpl_extra_col_slave_innodb.result
-Last_SQL_Error = 'Column 1 of table 'test.t5' cannot be converted
from type 'varchar(24 octets)' to type 'char(20 octets) character set
utf8mb4''
+Last_SQL_Error = 'Column 5 of table 'test.t5' cannot be converted
from type 'float' to type 'decimal(8,2)''
Why the change?
Is it because we can now convert from varchar() to char() ?
log_event_server.cc:
/* WRITE ROWS EVENTS store the bitmap in m_cols instead of m_cols_ai */
+ after_image= ((type == UPDATE_ROWS_EVENT) ? &m_cols_ai : &m_cols);
+ bitmap_intersect(table->write_set, after_image);
+ table->mark_columns_per_binlog_row_image();
+ if (type != WRITE_ROWS_EVENT && table->vfield)
+ table->mark_virtual_columns_for_write(0);
An observation: When we call mark_columns_per_binlog_row_image() above then
all bits in table->read_set is set and rpl_write_set == write_set.
This means that a lot of code in
table->mark_columns_per_binlog_row_image(); is not necessary.
Not much we can do about it.
Note that later in the code we always call
table->mark_columns_per_binlog_row_image();
We should make the second call conditional so that we do not call
it twice. We probably also do not need to call it in case of
rpl_data.copy_fields is set as all read, write and rpl_write_set bits all
already all set.
-----
+ /*
+ Store the original write_set in cond_set for
+ fill_extra_persistent_columns()
+ */
+ bitmap_copy(&table->cond_set, table->write_set);
this->slave_exec_mode= slave_exec_mode_options; // fix the mode
Missing. Noted.
+ if (rpl_data.copy_fields)
+ {
+ /* always full rows, all bits set */;
+ }
Fix comment to say:
/* We are exceutiong online alter table. Always full rows, all bits set */
Note no end ;
+ if (get_general_type_code() != UPDATE_ROWS_EVENT)
+ bitmap_copy(table->write_set, table->read_set);
Replace get_general_type_code() with type
+ if (!rpl_data.is_online_alter())
+ this->slave_exec_mode= (enum_slave_exec_mode) slave_exec_mode_options;
+ }
In the earlier code you are using:
if (rpl_data.copy_fields)
Please change all cases of 'if (rpl_data.copy_fields)' to use
if (rpl_data.is_online_alter())
(two cases)
Now code is using:
new(table_list) RPL_TABLE_LIST(&tmp_db_name, &tmp_tbl_name, TL_WRITE,
get_table_def(),
m_flags & TM_BIT_HAS_TRIGGERS_F);
Which is not calling
MDL_REQUEST_INIT(&mdl_request, MDL_key::TABLE, db.str, table_name.str,
mdl_type, MDL_TRANSACTION);
As we did in the old code using init_one_table().
Just wonder if it's safe.
----
rpl_record.cc
+static void prepare_null_field(Field *f, Unpack_record_state *st)
Would it not be better to make the functions using Unpack_record_state
class functions ?
This would be more efficent as the first argument to Unpack_record_state
is always the same, which makes things faster if we call Unpack_record_state
functions several times in a row with different arguments.
In void prepare_null_field():
#ifndef DBUG_OFF
/*
f->reset() may call store_value() to reset the value, for example
Field_new_decimal. store_value() has below assertion:
DBUG_ASSERT(marked_for_write_or_computed());
It asserts write bitmap must be set. That caused an assertion
failure for row images generated by FULL_NODUP mode.
The assertion is meaningless for unpacking a row image, so
the field is marked in write_set temporarily to avoid the
assertion failure.
*/
bool was_not_set= !bitmap_is_set(f->table->write_set, f->field_index);
if (was_not_set)
bitmap_set_bit(f->table->write_set, f->field_index);
#endif
f->reset();
#ifndef DBUG_OFF
if (was_not_set)
bitmap_clear_bit(f->table->write_set, f->field_index);
#endif
f->set_null();
A better way is to do above is the following:
DBUG_FIX_WRITE_SET(f);
f->reset();
f->set_null();
DBUG_RESTORE_WRITE_SET(f);
Remove the following comment:
This is not needed as the intention is that f->reset() is rarely
needed and doing that for every set_null() is extra overhead.
+ This could probably go into set_null() but doing so,
+ (i) triggers assertion in other parts of the code at
+ the moment; (ii) it would make us reset the field,
+ always when setting null, which right now doesn't seem
+ needed anywhere else except here.
+
+ TODO: maybe in the future we should consider moving
+ the reset to make it part of set_null. But then
+ the assertions triggered need to be
+ addressed/revisited.
+ */
----
+static bool unpack_field(const table_def *tabledef, Field *f,
+ Unpack_record_state *st, uint field_idx)
Rename field_idx to master_field_idx
+ DBUG_PRINT("debug", ("field: %s; metadata: 0x%x;"
+ " pack_ptr: %p; pack_ptr': %p; bytes: %d",
+ f->field_name.str, metadata, old_pack_ptr, st->pack_ptr,
+ (int) (st->pack_ptr - old_pack_ptr)));
Replace "debug" with "rpl_record" or "field".
One can tell debug to only print certain tags. For this to be useful
each DBUG_PRINT should have a tag related to the context.
This should be done to all DBUG_PRINT in rpl_record.cc
return static_cast<bool>(st->pack_ptr);
Better to use MY_TEST(st->pack_ptr) or st->pack_ptr != 0 as some
compilers may complain when casting a pointer to a bool.
-----
static void update_write_set_for_auto_filled_fields(TABLE *table,
Field **field_start_ptr)
{
DBUG_ENTER("update_write_set_for_auto_filled_fields");
if (!field_start_ptr || !*field_start_ptr)
DBUG_VOID_RETURN;
Better to move the check of field_start_pr in the caller.
(We have only one caller).
I checked the caller and it's a bit wrong as both
update_write_set_for_auto_filled_fields() and update_default_fields
requires that table->default_fields points
to something relevant.
I suggest we remove the above test for field_start_ptr and instead do in
the caller:
if (table->default_field && (rpl_data.is_online_alter() ||
LOG_EVENT_IS_WRITE_ROW(rgi->current_event->get_type_code())))
{
->
if (table->default_field && *table->default_field &&
rpl_data.is_online_alter() ||
LOG_EVENT_IS_WRITE_ROW(rgi->current_event->get_type_code()))
The identation above is also much clearer than the original code.
I also checked the code and as far as I can see, table->default field
is only set if there are default fields. In other words, the pointer
always points to something. This means we can replace testing
*table->default_field with an assert.
I am not sure why we call
update_write_set_for_auto_filled_fields() in unpack row for every row
It would be better if we do it one when we initalize the table maps once
in log_event_server and do not touch them after that.
Better to get this fixed now.
Same goes calling the function:
update_write_set_for_auto_filled_fields(table, table->vfield);
I did run mtr with the following patch, which fixes the above.
Please apply if you don't find anything strange with it:
diff --git a/sql/rpl_record.cc b/sql/rpl_record.cc
index d07c5ddba7b..173e8f949c3 100644
--- a/sql/rpl_record.cc
+++ b/sql/rpl_record.cc
@@ -320,12 +320,12 @@ static void convert_field(Field *result_field,
Field *conv_field)
of the table (e.g. table->default_field or
table->vfield).
*/
+
static void update_write_set_for_auto_filled_fields(TABLE *table,
Field **field_start_ptr)
{
DBUG_ENTER("update_write_set_for_auto_filled_fields");
- if (!field_start_ptr || !*field_start_ptr)
- DBUG_VOID_RETURN;
+ DBUG_ASSERT(*field_start_ptr);
Field **field_ptr, *field;
for (field_ptr= field_start_ptr; *field_ptr; ++field_ptr)
@@ -615,6 +615,9 @@ int unpack_row(const rpl_group_info *rgi, TABLE
*table, uint const master_cols,
*current_row_end = st.pack_ptr;
+ DBUG_ASSERT(!table->default_field || table->default_field[0]);
+ DBUG_ASSERT(!table->vfield || table->vfield[0]);
+
if (table->default_field && (rpl_data.is_online_alter() ||
LOG_EVENT_IS_WRITE_ROW(rgi->current_event->get_type_code())))
{
diff --git a/sql/sql_table.cc b/sql/sql_table.cc
index 5b0113982c3..670736aceb0 100644
--- a/sql/sql_table.cc
+++ b/sql/sql_table.cc
@@ -12716,7 +12716,12 @@ copy_data_between_tables(THD *thd, TABLE
*from, TABLE *to,
}
}
if (dfield_ptr)
- *dfield_ptr= NULL;
+ {
+ if (dfield_ptr == to->default_field)
+ to->default_field= 0; // No default fields left
+ else
+ *dfield_ptr= NULL;
+ }
if (order)
{
diff --git a/sql/table.cc b/sql/table.cc
index 07d71b47fdb..bdc6f55a969 100644
--- a/sql/table.cc
+++ b/sql/table.cc
@@ -1397,7 +1397,10 @@ bool parse_vcol_defs(THD *thd, MEM_ROOT
*mem_root, TABLE *table,
*vfield_ptr= 0;
if (dfield_ptr)
+ {
+ DBUG_ASSERT(table->default_field != dfield_ptr);
*dfield_ptr= 0;
+ }
if (check_constraint_ptr)
*check_constraint_ptr= 0;
---------------
This change:
RPL_TABLE_LIST *table_list= rgi->get_table_data(table);
->
Rpl_table_data rpl_data= *(RPL_TABLE_LIST*)table->pos_in_table_list;
Avoids looping over all locked tables to find the right one.
However in log_event_server.cc, we do:
Rpl_table_data rpl_data= *(RPL_TABLE_LIST*)table->pos_in_table_list;
Followed by:
RPL_TABLE_LIST *rpl_table= rgi->get_table_data(table);
I assume we can change this to:
RPL_TABLE_LIST *rpl_table= (RPL_TABLE_LIST*)table->pos_in_table_list;
I did test this with mtr (no failures):
@@ -5182,9 +5216,10 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi)
DBUG_PRINT_BITSET("debug", "Setting table's read_set from: %s", &m_cols);
{
- RPL_TABLE_LIST *rpl_table= rgi->get_table_data(table);
+ RPL_TABLE_LIST *rpl_table= (RPL_TABLE_LIST*)table->pos_in_table_list;
Log_event_type type= get_general_type_code();
DBUG_ASSERT(rpl_table);
+ DBUG_ASSERT(rpl_table == rgi->get_table_data(table));
bitmap_set_all(table->read_set);
bitmap_set_all(table->write_set);
----------
/*
A slave needs additional checks when unpacking a row than from the ONLINE
ALTER use case. The slave must account for its tables having either columns
in different positions, or with different types, than on the master.
*/
if (!rpl_data.is_online_alter())
{
Please move the comment to after 'else' as the comment is for online alter
table but the following code is for non alter table.
for (uint master_idx= 0; master_idx < master_cols; master_idx++)
was changed to at some point:
for (master_idx= 0; master_idx < master_cols; master_idx++)
Please add this back (in two places) to ensure that master_idx is not
used outside of the loops.
-----
Remove extra empty lines marked below (they don't make things more readable
and is not matching the style of the surrounding code:
if (!bitmap_is_set(cols, master_idx))
{
->
/*
Check if we need to update the conv_table_idx. A field is only added
to the conv_table when it exists on the slave.
conv_table_idx tracks the index of the field in the conversion
table, but
*/
if (!(tabledef->master_to_slave_error[master_idx])) // field
exists on slave
{
/*
Review-only-comment: Removed this assertion becaues it assumes that
the table->write_set is a superset of the read and write sets, but
it isn't. When an update row event is binlogged with
binlog_row_image=MINIMAL, the read and write sets can be disjoint.
As this is is only used for debug assertions, I decided to remove
the assertion altogether, rather than have a mode of callling the
function in to use either the read or write set.
*/
//DBUG_ASSERT(!bitmap_is_set(table->write_set,
// master_to_slave_map[master_idx]));
conv_table_idx++;
}
->
continue;
}
Also remove all double empty lines inside unpack_row().
I have tested what is the problem with the assert.
Logically, we should not update things on the slave that does not exists
on the master (execpt maybe in the cases of virtual columns, but that
can be tested).
I checked rpl.rpl_binlog_compress
The reason for the assert is that during updates we are first using
unpack_row() to find the primary key fields. This is failing as
the fields we are skipping over can later be updated.
Ok to remove the assert.
However this relations gives us an optimization opportunity:
In case of not online alter table, before:
for (; master_idx < master_cols; master_idx++)
we could do
master_cols= my_find_last_bit(cols);
This would allow us to skip all not used fields, which for the read part
would speed up the loop notable for tables with many fields.
I will add my_find_last_bit() to main (I already have a patch)
st.pack_ptr+=
tabledef->calc_field_size(master_idx, (uchar *) st.pack_ptr);
The correct intendation is:
st.pack_ptr+= tabledef->calc_field_size(master_idx,
(uchar *) st.pack_ptr);
-------
if (master_reclength)
{
if (result_field)
*master_reclength = (ulong)(result_field->ptr - table->record[0]);
else
*master_reclength = table->s->reclength;
}
master_reclength is not used anywhere (usage was removed 2007). We can
remove this here and all other places. This also allows us to move
result_field declaration inside the for loop.
I will do that in main shortly (should be a separate patch)
This also solves a possible bug where 'result_field' is not the last field
in the table, which makes 'master_reclengh' value hard to understand.
-----
I am not reviewing the 'Online alter table part, except noticing two
things that should be fixed:
#ifndef DBUG_OFF
bool result=
#endif
unpack_field(tabledef, f, &st, master_idx);
DBUG_ASSERT(result);
We shouldn't have #ifndef in the middle of the code (makes things hard to read)
Better to use:
bool result __attribute__((unused))= unpack_field(...);
----
for (const auto *copy=rpl_data.copy_fields;
->
for (const Copy_field *copy= rpl_data.copy_fields;
We should remove auto (lazy, and not good when using humans or AI for reviews)
-----
Regards,
Monty
_______________________________________________
commits mailing list -- [email protected]
To unsubscribe send an email to [email protected]
