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@mariadb.org