maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei,

On Wed, Mar 2, 2022 at 10:33 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> 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.

Added.

>
> > > 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 don't think it is a good idea to intermix this semantic into these
variables/flags. log_current_statement was designed specifically for
this.

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

That is pretty normal. It is the business logic that is complex and
therefore its realization should be complex too. Compressing many
semantics into single control variables only makes development harder.

>
> >
> > > > +
> > > >      my_ok(thd, 0);
> > > >      DBUG_RETURN(0);
> > > >    }
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
@midenok