Hi, Nikita,
On Jul 08, Nikita Malyavin wrote:
> revision-id: 3e2d297830b (mariadb-10.6.1-511-g3e2d297830b)
> parent(s): c1feb8c3c32
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2022-07-05 15:20:04 +0300
> message:
>
> MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter with long unique
>
> 1. ER_KEY_NOT_FOUND
> Some virtual columns were not updated because were not included in read_set
> to the moment of calculation.
>
> In Row_logs_event::do_apply_event some fields are excluded based on m_cols
> value, the number of replicated rows.
the number of replicated *columns*
> We can't rely on this. Basically, we'd be satisfied, if all columns were
> just set, at least for now.
>
> 2. ER_LOCK_WAIT_TIMEOUT
> This is a long unique specific problem.
>
> Sometimes, lookup_handler is created for to->file. To properly free it,
> ha_reset should be called. It is usually done by calling
> close_thread_table, but ALTER TABLE makes it differently. Hence, a single
> ha_reset call is added to mysql_alter_table.
>
> Also, the lifetime of lookup_handler is corrected: it's been allocated on
> thd->mem_root, which in case of online alter
> is event's mem_root, and can vary other ways.
> It is changed to table->mem_root, which is more appropriate:
> ha_reset is called only when table is closed, or in a few additional cases,
> which correspond to a statement end. So memory leaks are unlikely here.
>
> diff --git a/sql/handler.cc b/sql/handler.cc
> index d71c0fc83a3..fbebfd27203 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -3212,7 +3212,7 @@ int handler::create_lookup_handler()
> handler *tmp;
> if (lookup_handler != this)
> return 0;
> - if (!(tmp= clone(table->s->normalized_path.str, table->in_use->mem_root)))
> + if (!(tmp= clone(table->s->normalized_path.str, &table->mem_root)))
that's wrong, the lifetime of a lookup handler is one statement, it's
destroyed at the end. You cannot keep allocating new handlers on the
table->mem_root for every statement.
> return 1;
> lookup_handler= tmp;
> return lookup_handler->ha_external_lock(table->in_use, F_RDLCK);
> diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc
> index 89ca2a41937..2c19bf67586 100644
> --- a/sql/log_event_server.cc
> +++ b/sql/log_event_server.cc
> @@ -6007,6 +6007,11 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi)
> if (m_width == table->s->fields && bitmap_is_set_all(&m_cols))
> set_flags(COMPLETE_ROWS_F);
>
> + Rpl_table_data rpl_data{};
> + if (rgi) rgi->get_table_data(table, &rpl_data);
> +
> + if (!rpl_data.copy_fields)
> + {
> /*
> Set tables write and read sets.
>
> @@ -6027,17 +6027,19 @@ int Rows_log_event::do_apply_event(rpl_group_info
> *rgi)
> bitmap_set_all(table->read_set);
> if (get_general_type_code() == DELETE_ROWS_EVENT ||
> get_general_type_code() == UPDATE_ROWS_EVENT)
> bitmap_intersect(table->read_set,&m_cols);
>
> bitmap_set_all(table->write_set);
>
> /* WRITE ROWS EVENTS store the bitmap in m_cols instead of m_cols_ai */
> MY_BITMAP *after_image= ((get_general_type_code() == UPDATE_ROWS_EVENT) ?
> &m_cols_ai : &m_cols);
> bitmap_intersect(table->write_set, after_image);
>
> this->slave_exec_mode= slave_exec_mode_options; // fix the mode
> + }
I'm sorry, I don't quite understand. "some fields are excluded based on
m_cols value" - what columns were excluded? What was the value of m_cols
here?
> table->rpl_write_set= table->write_set;
>
> // Do event specific preparations
> error= do_before_row_operations(rli);
> @@ -6045,8 +6052,6 @@ int Rows_log_event::do_apply_event(rpl_group_info *rgi)
> extra columns on the slave. In that case, do not force
> MODE_NO_AUTO_VALUE_ON_ZERO.
> */
> - Rpl_table_data rpl_data{};
> - if (rgi) rgi->get_table_data(table, &rpl_data);
> sql_mode_t saved_sql_mode= thd->variables.sql_mode;
> if (!is_auto_inc_in_extra_columns())
> thd->variables.sql_mode= (rpl_data.copy_fields ? saved_sql_mode : 0)
> diff --git a/sql/sql_table.cc b/sql/sql_table.cc
> index 29c4d09cd39..a3efcdd9785 100644
> --- a/sql/sql_table.cc
> +++ b/sql/sql_table.cc
> @@ -11011,6 +11011,9 @@ do_continue:;
> !(table->file->ha_table_flags() & HA_REUSES_FILE_NAMES) &&
> !(new_table->file->ha_table_flags() &
> HA_REUSES_FILE_NAMES));
> +
> + // Close lookup_handler.
> + new_table->file->ha_reset();
please, put it not here, but inside the following
thd->drop_temporary_table() call. It'll also fix Sanja's bug MDEV-26433.
And please coordinate with him to put it on the same place so that git
would detect a conflict and we could merge it to have only one reset
call in that method, not two.
> /*
> Close the intermediate table that will be the new table, but do
> not delete it! Even though MERGE tables do not have their children
> @@ -11854,8 +11857,11 @@ copy_data_between_tables(THD *thd, TABLE *from,
> TABLE *to,
>
> // We'll be filling from->record[0] from row events
> bitmap_set_all(from->write_set);
> - // We restore bitmaps, because update event is going to mess up with
> them.
> + // Use all columns. UPDATE/DELETE events reset read_set and write_set to
> + // def_*_set after each row operation, so using all_set won't work.
> to->default_column_bitmaps();
> + bitmap_set_all(&to->def_read_set);
> + bitmap_set_all(&to->def_write_set);
>
> end_read_record(&info);
> init_read_record_done= false;
>
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
_______________________________________________
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
