maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey,

On Feb 15, Aleksey Midenkov wrote:
> > On Feb 12, Aleksey Midenkov wrote:
> > > commit 0402f6dcb67
> > > Author: Aleksey Midenkov <midenok@xxxxxxxxx>
> > > Date:   Tue Feb 8 22:54:03 2022 +0300
> > >
> > > diff --git a/sql/sql_delete.cc b/sql/sql_delete.cc
> > > index 0403d6e73c3..06068290247 100644
> > > --- a/sql/sql_delete.cc
> > > +++ b/sql/sql_delete.cc
> > > @@ -507,6 +508,18 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND *conds,
> > >      if (thd->lex->describe || thd->lex->analyze_stmt)
> > >        goto produce_explain_and_leave;
> > >
> > > +    if (thd->log_current_statement)
> > > +    {
> > > +      thd->reset_unsafe_warnings();
> > > +      if (thd->binlog_query(THD::STMT_QUERY_TYPE,
> > > +                            thd->query(), thd->query_length(),
> > > +                            transactional_table, FALSE, FALSE, 0) > 0)
> > > +      {
> > > +        my_error(ER_ERROR_ON_WRITE, MYF(0), "binary log", -1);
> > > +        DBUG_RETURN(1);
> > > +      }
> > > +    }
> >
> > perhaps, move this block into a function? It's repeated four times here.
> >
> 
> Moved.

Thanks!
The new method, binlog_for_noop_dml() looks very puzzling.

I understand what it does, for now, but somebody who wasn't involved in
this bugfixing, likely won't have any idea.
The name is correct, I agree, it says what, but it doesn't explain why.

Could you add a comment there? To say that "noop dml" is "an
insert(odku)/update/delete that doesn't change the table" and that
it is normally not logged, but needs to be logged if it auto-created a
partition as a side effect.

> > by the way, you don't seem to have a test for a zero-rows insert.
> > That is INSERT ... SELECT with an impossible where or limit 0.
> >
> > In fact, you can have a zero-row insert with a normal insert
> > INSERT t1 VALUES (1); if this (1) is a unique key violation.
> > I believe your code handles this case, thought, just a test case is
> > missing.
> 
> INSERT does not trigger history. But INSERT .. ODKU does. Added test
> cases for INSERT .. ODKU and INSERT .. SELECT .. ODKU. That required
> to add THD::vers_created_partitions as log_current_statement is also
> used in CREATE TABLE (and select_insert::prepare_eof() is used for
> CREATE TABLE .. SELECT) and that broke some replication tests:
> 
> rpl.create_or_replace_mix
> rpl.create_or_replace_row
> rpl.create_or_replace_statement

Indeed, I see.

Can you use OPTION_KEEP_LOG instead?
Or transaction->stmt.modified_non_trans_table ?

I hate it that there're so many ways to force binlogging a statement,
and they're all, of course, are subtly different.

> 
> > > +
> > >      my_ok(thd, 0);
> > >      DBUG_RETURN(0);
> > >    }

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx