Hi, Nikita, On May 24, Nikita Malyavin wrote:

revision-id: 56e2cd20ed3 (mariadb-11.0.1-114-g56e2cd20ed3) parent(s): 47f5c7ae7e4 author: Nikita Malyavin committer: Nikita Malyavin timestamp: 2023-05-10 01:49:42 +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.

The fix makes the events replicate only when the handler is a root handler. We will try to *guess* this by comparing it to table->file. The same approach is used in the MDEV-21540 fix, 231feabd. The assumption is made, that the row methods are only called for table->file (and never for a cloned handler), hence the assertions are added in ha_innobase and ha_myisam to make sure that this is true at least for those engines

Also closes MDEV-31040, however the test is not included, since we have no convenient way to construct a deterministic version.

diff --git a/sql/handler.cc b/sql/handler.cc index 3fcbb5171d6..d8aca5bc80b 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -7280,7 +7280,7 @@ 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 && is_root_handler())) error= binlog_log_row_online_alter(table, before_record, after_record, log_func); #endif // HAVE_REPLICATION @@ -7800,7 +7800,7 @@ int handler::ha_write_row(const uchar *buf) DBUG_RETURN(error);

/* - NOTE: this != table->file is true in 3 cases: + NOTE: is_root_handler() is false in 3 cases:

may be move this comment closer to is_root_handler() ?

1. under copy_partitions() (REORGANIZE PARTITION): that does not require long unique check as it does not introduce new rows or new index. @@ -7810,7 +7810,7 @@ int handler::ha_write_row(const uchar *buf) 3. under ha_mroonga::wrapper_write_row() */

- if (table->s->long_unique_table && this == table->file) + if (table->s->long_unique_table && is_root_handler()) { DBUG_ASSERT(inited == NONE || lookup_handler != this); if ((error= check_duplicate_long_entries(buf))) @@ -7862,13 +7862,13 @@ int handler::ha_update_row(const uchar *old_data, const uchar *new_data) error= ha_check_overlaps(old_data, new_data);

/* - NOTE: this != table->file is true under partition's ha_update_row(): + NOTE: is_root_handler() is false under partition's ha_update_row(): check_duplicate_long_entries_update() was already done by ha_partition::ha_update_row(), no need to check it again for each single partition. Same applies to ha_mroonga wrapper. */

- if (!error && table->s->long_unique_table && this == table->file) + if (!error && table->s->long_unique_table && is_root_handler()) error= check_duplicate_long_entries_update(new_data); table->status= saved_status;

@@ -7913,6 +7913,12 @@ int handler::ha_update_row(const uchar *old_data, const uchar *new_data) return error; }

+ +bool handler::is_root_handler() const +{ + return this == table->file; +}

better make it inline in handler.h

+ /* Update first row. Only used by sequence tables */ diff --git a/sql/handler.h b/sql/handler.h index fb0eba83c38..9b903e7ebce 100644 --- a/sql/handler.h +++ b/sql/handler.h @@ -3502,6 +3502,10 @@ class handler :public Sql_alloc return extra(HA_EXTRA_NO_KEYREAD); }

+protected: + bool is_root_handler() const; + +public: int check_collation_compatibility(); int check_long_hash_compatibility() const; int ha_check_for_upgrade(HA_CHECK_OPT *check_opt); diff --git a/storage/innobase/handler/ha_innodb.cc b/storage/innobase/handler/ha_innodb.cc index f035437b1d7..94d9da3486b 100644 --- a/storage/innobase/handler/ha_innodb.cc +++ b/storage/innobase/handler/ha_innodb.cc @@ -7750,6 +7750,8 @@ ha_innobase::write_row(

DBUG_ENTER("ha_innobase::write_row");

+ DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr);

instead of putting this assert into every handler's implementation and in many methods, why not to have it in handler ha_ methods or may be even in the is_root_handler itself? You can compare this->ht with table->file->ht. For example, like DBUG_ASSERT(this == table->file || this->ht != table->file->ht);

+ trx_t* trx = thd_to_trx(m_user_thd);

/* Validation checks before we commence write_row operation. */ @@ -8479,6 +8481,8 @@ ha_innobase::update_row(

DBUG_ENTER("ha_innobase::update_row");

+ DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); + if (is_read_only()) { DBUG_RETURN(HA_ERR_TABLE_READONLY); } else if (!trx_is_started(trx)) { @@ -8653,6 +8657,8 @@ ha_innobase::delete_row(

DBUG_ENTER("ha_innobase::delete_row");

+ DBUG_ASSERT(is_root_handler() || table->file->ht != innodb_hton_ptr); + if (is_read_only()) { DBUG_RETURN(HA_ERR_TABLE_READONLY); } else if (!trx_is_started(trx)) { diff --git a/storage/myisam/ha_myisam.cc b/storage/myisam/ha_myisam.cc index 4057c3ea859..b7fadd86a23 100644 --- a/storage/myisam/ha_myisam.cc +++ b/storage/myisam/ha_myisam.cc @@ -983,6 +983,7 @@ int ha_myisam::close(void)

int ha_myisam::write_row(const uchar *buf) { + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); /* If we have an auto_increment column and we are writing a changed row or a new row, then update the auto_increment value in the record. @@ -1957,11 +1958,13 @@ bool ha_myisam::is_crashed() const

int ha_myisam::update_row(const uchar *old_data, const uchar *new_data) { + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); return mi_update(file,old_data,new_data); }

int ha_myisam::delete_row(const uchar *buf) { + DBUG_ASSERT(is_root_handler() || table->file->ht != ht); return mi_delete(file,buf); }

Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org