On Tue, 8 Aug 2023 at 21:09, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
On Aug 08, Nikita Malyavin wrote:
yes, extra safety can be worth it. I'd also add an assertion on top of that:
--- a/sql/sql_table.cc +++ b/sql/sql_table.cc @@ -12181,6 +12181,7 @@ copy_data_between_tables(THD *thd, TABLE *from, TABLE *> if (error) from->s->tdc->flush_unused(1); // to free the binlog to->pos_in_table_list= NULL; // Safety + DBUG_ASSERT(thd->lex->sql_command == SQLCOM_ALTER_TABLE); + thd->lex->sql_command= SQLCOM_ALTER_TABLE; } else if (online) // error was on copy stage {
That is, if we'll catch it while testing, we'll fix it. And users will be protected form the possible corner cases we didn't think of
Well, that's not exactly what I meant. I meant that row event changing thd->lex->sql_command might be intentional and not something that needs to be fixed.
So just reset it back to SQLCOM_ALTER_TABLE in copy_data_between_tables() and that's it. Without assert and without avoiding SQLCOM_REPLACE.
It doesn't just avoid sql_command reset: look through the slave_exec_mode usage -- it does a few things that can have side-effects. Better to avoid it altogether and keep our execution flow away from interactions with replication switches. The assertion I've added doesn't fail, so no, only IDEMPOTENT inserts do this. -- Yours truly, Nikita Malyavin