maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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 <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.

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 security@xxxxxxxxxxx