"For MDEV-36290 (improved support of replication between tables of
different structure) I've addressed your review comments, as well as a
couple bugs found in test in the latest branch main-MDEV-36290. The
last 3 commits are incremental from the patch you reviewed. Do you
want to take a look?"
Here is the review for each patch in main-MDEV-36290
MDEV-36290: Address review comments (and some other cleanups found)
- Commit comment looks very good. Great that you addressed my concerns!
Relevant Comments from review script:
// sql/rpl_utility_server.cc:1377
- continue; // ok that field
did not exists
+ continue; // ok that field
did not exist
Rest of the suggestion where wrong
A funny note, I called review.sh accidentally with inverse argument so
the LLM thought your new code was the old code.
The main comments where:
* **Good Architectural Change:** Moving the `write_set` update logic
into `unpack_row` is a clear improvement. It's more localized,
executed only when needed, and makes the data flow easier to
understand.
* **Bug Fix:** The PR correctly identifies and resolves an assertion
failure (`marked_for_write_or_computed()`) when applying row events
with `NULL` values to fields that are not in the `write_set`. The
solution of temporarily marking the field for writing is appropriate.
After the above, the LLM got even more confused.
------
log_event_server.cc
- if (ptr->create_column_mapping(rgi))
- {
- thd->is_slave_error= 1;
- error= ERR_BAD_TABLE_DEF;
- goto err;
- }
ptr->create_column_mapping(rgi);
Note the create_column_mapping can fail if this fails:
m_tabledef.master_column_name[col]= (char *) alloc_root(
rgi->thd->mem_root, field_name_sz * sizeof(char) + 1);
Better to keep the value as bool and do a goto err.
Error in thd is will automatically be set.
You can of course also set error ER_OUTOFMEMORY as the function returns error.
------
rpl_record.cc:
void prepare_null_field(Field *f)
Add #ifndef DBUG_OFF around
MY_BITMAP *write_set= f->table->write_set;
This is to avoid warnings about not used variables in not debug compilations.
An even better fix would be to add write_set as an argument to the macro
(I don't like macros that uses local variables in a hidden manner):
#define DBUG_FIX_WRITE_SET(f) bool _write_set_fixed=
!bitmap_fast_test_and_set(write_set, (f)->field_index)
->
#define DBUG_FIX_WRITE_SET(f,write_set) bool _write_set_fixed=
!bitmap_fast_test_and_set(write_set, (f)->field_index)
This way you don't need the write_set variable.
int unpack_row()
+ DBUG_ASSERT(!table->default_field || table->default_field[0]);
+ DBUG_ASSERT(!table->vfield || table->vfield[0]);
+
+ if (table->default_field && *(table->default_field) &&
+ (rpl_data.is_online_alter() ||
+ LOG_EVENT_IS_WRITE_ROW(rgi->current_event->get_type_code())))
You don't need to test for *(table->default_field) anymore.
- if (table->vfield)
+ if (table->vfield && *(table->vfield))
Same here. Testing for table->vfield is enough.
-----
sql_table.cc:
if (dfield_ptr)
- *dfield_ptr= NULL;
+ {
+ if (dfield_ptr == to->default_field)
+ to->default_field= 0; // No default fields left
+ else
+ *dfield_ptr= NULL;
+ }
Add the following comments:
if (dfield_ptr)
{
if (dfield_ptr == to->default_field)
to->default_field= 0; // No default fields in table
else
*dfield_ptr= NULL; // Mark end of default field pointers
}
RPL_TABLE_LIST::create_column_mapping
size_t field_name_sz= master_col_name_cppstr.size();
m_tabledef.master_column_name[col]= (char *) alloc_root(
rgi->thd->mem_root, field_name_sz * sizeof(char) + 1);
strncpy(m_tabledef.master_column_name[col],
master_col_name_cppstr.c_str(), field_name_sz);
m_tabledef.master_column_name[col][field_name_sz] = '\0';
The above can be replace by:
m_tabledef.master_column_name[col]=
rgi->thd->strmake(m_tabledef.master_column_name[col].ptr(),
m_tabledef.master_column_name[col].size());
if (!m_tabledef.master_column_name[col])
return 1; // Out of memory
It would also be nice if you could replace at some point std::string with
LEX_CSTRING, which is more efficent. This also would allow us to get rid
if calling c_str() in a lot of places.
Note that find_field_by_name[] does not require string to be 0 terminated.
----
table.cc:
if (dfield_ptr)
+ {
+ DBUG_ASSERT(table->default_field != dfield_ptr);
*dfield_ptr= 0;
+ }
->
if (dfield_ptr)
{
/* table->default_field == NULL if no default fields */
DBUG_ASSERT(table->default_field != dfield_ptr);
*dfield_ptr= 0; // Mark end of default field pointers
}
--------------------
commit cfe91d97c82221a42eb1b1e26ec168a9a8a1ec1c
Author: Brandon Nesterenko <[email protected]>
Date: Fri Oct 10 12:53:44 2025 -0600
MDEV-37737: SIGABRT upon executing BINLOG statement
ok
commit 16c8bcc09a22709fdb770ee267317dac1e033984 (HEAD ->
main-MDEV-36290, origin/main-MDEV-36290)
Author: Brandon Nesterenko <[email protected]>
Date: Fri Oct 10 13:09:07 2025 -0600
MDEV-37836: ASAN: global-buffer-overflow in
Sys_var_typelib::Sys_var_typelib on MDEV-36290 branch on server
startup
ok
-----
Regards,
Monty
_______________________________________________
commits mailing list -- [email protected]
To unsubscribe send an email to [email protected]
