Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi Sergei,
On Wed, Mar 2, 2022 at 10:33 PM Sergei Golubchik wrote:
>
> Hi, Aleksey,
>
> On Feb 15, Aleksey Midenkov wrote:
> > > On Feb 12, Aleksey Midenkov wrote:
> > > > commit 0402f6dcb67
> > > > Author: Aleksey Midenkov
> > > > 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 secur...@mariadb.org
--
@midenok
___
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
Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi, Aleksey,
On Feb 15, Aleksey Midenkov wrote:
> > On Feb 12, Aleksey Midenkov wrote:
> > > commit 0402f6dcb67
> > > Author: Aleksey Midenkov
> > > 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 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
Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi Sergei!
Updated branch preview-10.8-MDEV-17554-auto-create-partition
On Sat, Feb 12, 2022 at 2:31 AM Sergei Golubchik wrote:
>
> Hi, Aleksey,
>
> This was quite good, thanks!
>
> Many cases covered, not just ROLLBACK, but also LIMIT 0 and
> impossible WHERE.
>
> See a couple of comments below. Nothing big.
>
> On Feb 12, Aleksey Midenkov wrote:
> > commit 0402f6dcb67
> > Author: Aleksey Midenkov
> > 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.
> 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
> > +
> > my_ok(thd, 0);
> > DBUG_RETURN(0);
> >}
> > @@ -932,8 +958,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list,
> > COND *conds,
> >else
> > errcode= query_error_code(thd, killed_status == NOT_KILLED);
> >
> > - ScopedStatementReplication scoped_stmt_rpl(
> > - table->versioned(VERS_TRX_ID) ? thd : NULL);
> > + StatementBinlog stmt_binlog(thd, table->versioned(VERS_TRX_ID) ||
> > +
> > thd->binlog_need_stmt_format(transactional_table));
>
> I know that's not part of this patch, but still. Just trying to understand.
> Why did it change to statement-based if VERS_TRX_ID?
Because we don't need replicated TRX_ID as they are different on
slave. The related commit is d998da03069 and the issue is
https://github.com/tempesta-tech/mariadb/issues/234
>
> >/*
> > [binlog]: If 'handler::delete_all_rows()' was called and the
> > storage engine does not inject the rows itself, we replicate
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
@midenok
___
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
Re: [Maria-developers] 0402f6dcb67: MDEV-25477 Auto-create breaks replication when triggering event was not replicated
Hi, Aleksey,
This was quite good, thanks!
Many cases covered, not just ROLLBACK, but also LIMIT 0 and
impossible WHERE.
See a couple of comments below. Nothing big.
On Feb 12, Aleksey Midenkov wrote:
> commit 0402f6dcb67
> Author: Aleksey Midenkov
> 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.
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.
> +
> my_ok(thd, 0);
> DBUG_RETURN(0);
>}
> @@ -932,8 +958,8 @@ bool mysql_delete(THD *thd, TABLE_LIST *table_list, COND
> *conds,
>else
> errcode= query_error_code(thd, killed_status == NOT_KILLED);
>
> - ScopedStatementReplication scoped_stmt_rpl(
> - table->versioned(VERS_TRX_ID) ? thd : NULL);
> + StatementBinlog stmt_binlog(thd, table->versioned(VERS_TRX_ID) ||
> +
> thd->binlog_need_stmt_format(transactional_table));
I know that's not part of this patch, but still. Just trying to understand.
Why did it change to statement-based if VERS_TRX_ID?
>/*
> [binlog]: If 'handler::delete_all_rows()' was called and the
> storage engine does not inject the rows itself, we replicate
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
