Hi Sergei!
Updated branch preview-10.8-MDEV-17554-auto-create-partition
On Sat, Feb 12, 2022 at 2:31 AM Sergei Golubchik <s...@mariadb.org> 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 <mide...@gmail.com>
> > 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
