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