maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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