Re: [Maria-developers] 56e2cd20ed3: MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table
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
Hello! On Wed, 24 May 2023 at 18:32, Sergei Golubchik <serg@mariadb.org> wrote:
@@ -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?
We can only put this assertion to a leaf handler. The assertion itself means: "A it's a leaf handler, then it's either a root handler as well, or a root hton is different".
You can compare this->ht with table->file->ht. For example, like
DBUG_ASSERT(this == table->file || this->ht != table->file->ht);
This'll mean that there can't be more than two layers of handlers. I suspect I'm able to move the assertion to a more generic place, like is_root_handler, but I'd have to check whether a handler's hton is a "certainly leaf" one by comparing it with ha_resolve_by_name outputs for "myisam" and "innodb". -- Yours truly, Nikita Malyavin
Hi, Nikita, As far as I'm concerned, not having an assert there is quite fine too :) On May 27, Nikita Malyavin wrote:
I suspect I'm able to move the assertion to a more generic place, like is_root_handler, but I'd have to check whether a handler's hton is a "certainly leaf" one by comparing it with ha_resolve_by_name outputs for "myisam" and "innodb".
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
On Wed, 24 May 2023 at 18:32, Sergei Golubchik <serg@mariadb.org> wrote:
+bool handler::is_root_handler() const +{ + return this == table->file; +}
better make it inline in handler.h
Can't 😓 TABLE is incomplete in handler.h.
-- Yours truly, Nikita Malyavin
Hi, Nikita, See, e.g. handler::is_clustering_key() or handler::ha_rnd_pos_by_record() On May 28, Nikita Malyavin wrote:
On Wed, 24 May 2023 at 18:32, Sergei Golubchik <serg@mariadb.org> wrote:
+bool handler::is_root_handler() const +{ + return this == table->file; +}
better make it inline in handler.h
Can't 😓 TABLE is incomplete in handler.h.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hello Sergei! I think I've found a way that will satisfy both of us, and will also close my claim regarding the need for assertion. See commit 0bcbec79 <https://github.com/MariaDB/server/commit/0bcbec79cf73c04f758bfb2c80d67c0f72ed1257> . Comparing htons seems to work also for REORGANIZE PARTITION, and will allow making changes from handler clones! On Sun, 28 May 2023 at 12:36, Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Nikita,
See, e.g. handler::is_clustering_key() or handler::ha_rnd_pos_by_record()
On May 28, Nikita Malyavin wrote:
On Wed, 24 May 2023 at 18:32, Sergei Golubchik <serg@mariadb.org> wrote:
+bool handler::is_root_handler() const +{ + return this == table->file; +}
better make it inline in handler.h
Can't 😓 TABLE is incomplete in handler.h.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin
participants (2)
-
Nikita Malyavin -
Sergei Golubchik