maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita,

On May 05, Nikita Malyavin wrote:
> revision-id: da5a72f32d4 (mariadb-11.0.1-114-gda5a72f32d4)
> parent(s): 0d6584c019c
> author: Nikita Malyavin
> committer: Nikita Malyavin
> timestamp: 2023-05-01 01:15:41 +0300
> message:
> 
> MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table
> 
> The row events were applied "twice": once for the ha_partition, and
> one more time for the underlying storage engine.
> 
> There's no such problem in binlog/rpl, because
> ha_partiton::row_logging is normally set to false.

s/ha_partiton/ha_partition/

> The proposed fix adds table->skip_online_logging field, preventing the
> underlying engines from replicating online events. Only upper layer
> engine should do this.
> 
> table->skip_online_logging is initialized to 0 together with the table
> itself. Normally it is always 0, except for the intermediate
> overriding part, once it is checked in handler::binlog_log_row.
> 
> Note on MDEV-31040 (Server crashes in MYSQL_LOG::is_open upon ALTER on
> partitioned table):
> First the solution was to override table->s->online_alter_binlog, and
> set it to NULL during the partitioning operation, which was
> ridiculously wrong: no-one is supposed to modify that field outside
> ALTER! That field write access is protected by EXCLUSIVE metadata
> lock.

I don't think you need to list your different (non-working) attemps to
fix the bug in the commit comment. Just explain the fix that works, not
the fix that doesn't.

> In comparison, table->skip_online_logging is a connection-local field.
> 
> The test is a non-deterministic one. It wasn't convenient to construct
> a deterministic test here, besides for the case that will never be
> repeated.

Is it still the case? Your test in alter_table_online_debug.test with
debug_sync - is it non-deterministic?

> diff --git a/sql/handler.cc b/sql/handler.cc
> index 3fcbb5171d6..a7e229e8f8d 100644
> --- a/sql/handler.cc
> +++ b/sql/handler.cc
> @@ -7280,7 +7280,8 @@ int handler::binlog_log_row(const uchar *before_record,
>                                      log_func, row_logging_has_trans);
>  
>  #ifdef HAVE_REPLICATION
> -  if (unlikely(!error && table->s->online_alter_binlog))
> +  if (unlikely(!error && table->s->online_alter_binlog &&
> +               !table->skip_online_logging))

this doesn't need any juggling with skip_online_logging.
This problem is already solved for, say, long uniques. You can use

   this == table->file

condition to filter out individual partitions and have your
binlog_log_row_online_alter called only for the main ha_partition
handler.

>      error= binlog_log_row_online_alter(table, before_record, after_record,
>                                         log_func);
>  #endif // HAVE_REPLICATION

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