On Tue, 8 Aug 2023 at 21:09, Sergei Golubchik <s...@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
_______________________________________________
developers mailing list -- developers@lists.mariadb.org
To unsubscribe send an email to developers-le...@lists.mariadb.org
