1. 15 Aug, 2023 40 commits
    • Nikita Malyavin's avatar
      MDEV-31043 ER_KEY_NOT_FOUND upon concurrent ALTER and transaction · 0695f7dd
      Nikita Malyavin authored
      Non-transactional engines changes are visible immediately the row operation
      succeeds, so we should apply the changes to the online buffer immediately
      after the statement ends.
      0695f7dd
    • Nikita Malyavin's avatar
      MDEV-31033 ER_KEY_NOT_FOUND upon online COPY ALTER on a partitioned table · c76072db
      Nikita Malyavin authored
      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.
      c76072db
    • Nikita Malyavin's avatar
      MDEV-30945 RPL tests are failing with MSAN use-of-uninitialized-value · 0775c7bd
      Nikita Malyavin authored
      ...in bitmap_intersect
      
      m_cols_ai was accessed during the Delete event, however this field is only
      related to Updates.
      
      Moving it to Update_rows_event would require too much effort. So instead:
      * Only access m_cols_ai in Update events (conditional branch is added in
        Rows_log_event::do_add_row_data)
      * Clean up m_cols_ai operations in Rows_log_event constructor.
        m_cols_ai.bitmap is first set to NULL, indicating invalid event.
        Then it is initialized:
        -> For Update events, a new bitmap is created.
        -> For other events, debug mode, m_cols_ai.bitmap is set to 1, indicating
           that the value is correct, but it shouldn't be accessed. To make sure
           we'll have a failure, n_bits is also set to 1.
        -> In release mode, m_cols_ai mirrors m_cols, providing extra safety
           in production.
      0775c7bd
    • Nikita Malyavin's avatar
      clean up Rows_log_event virtual methods · 2ce24405
      Nikita Malyavin authored
      Modernize declaration to C++11.
      Add missing const.
      Convert TYPE_CODE to constexpr.
      2ce24405
    • Nikita Malyavin's avatar
      MDEV-30891 Assertion `!table->versioned(VERS_TRX_ID)' failed · e1f5c58a
      Nikita Malyavin authored
      Assertion `!table->versioned(VERS_TRX_ID)' failed in
      Write_rows_log_event::binlog_row_logging_function during ONLINE ALTER.
      
      trxid-versioned tables can't be replicated.
      ONLINE ALTER will also be forbidden for these tables.
      e1f5c58a
    • Nikita Malyavin's avatar
      add partition test · 5361b870
      Nikita Malyavin authored
      5361b870
    • Nikita Malyavin's avatar
      MDEV-30985 Replica stops with error on ALTER ONLINE with Geometry Types · 5f206259
      Nikita Malyavin authored
      The bug is inherent for row-based replication as well.
      To reproduce, a virtual (not stored) field of a blob type computed from
      another field of a different blob type is required.
      
      The following happens during an update or delete row event:
      1. A row is unpacked.
      2. Virtual fields are updated. Field b1 stores the pointer in
         Field_blob::value and references it in table->record[0].
      3. record[0] is stored to record[1] in Rows_log_event::find_row.
      4. A new record is fetched from handler. (e.g. ha_rnd_next)
      5. Virtual columns are updated (only non-stored).
      6. Field b1 receives new value. Old value is deallocated
         (Field_blob::val_str).
      7. record_compare is called. record[0] and record[1] are compared.
      8. record[1] contains a reference to a freed value.
      
      record_compare is used in replication to find a matching record for update
      or delete. Virtual columns that are not stored should be definitely skipped
      both for correctness, and for this bug fix.
      
      STORED virtual columns, on the other hand, may be required and shouldn't be
      skipped. Stored columns are not affected, since they are not updated after
      handler's fetch.
      5f206259
    • Nikita Malyavin's avatar
      MDEV-30924 Server crashes in MYSQL_LOG::is_open upon ALTER vs FUNCTION · 3ad0e7ed
      Nikita Malyavin authored
      ASAN showed use-after-free in binlog_online_alter_end_trans, during
      running through thd->online_alter_cache_list.
      
      In online_alter_binlog_get_cache_data, new_cache_data was allocated on
      thd->mem_root, in case of autocommit=1, but this mem_root could be freed
      in sp_head::execute, upon using stored functions.
      
      It appears that thd->transaction->mem_root exists even in single-stmt
      transaction mode (i.e autocommit=1), so it can be used in all cases.
      This mem_root will remain valid till the end of transaction, including
      commit phase.
      3ad0e7ed
    • Nikita Malyavin's avatar
      MDEV-30925 Assertion failed in translog_write_record in ONLINE ALTER + Aria · 6b35d6a9
      Nikita Malyavin authored
      This is the corner case of ONLINE ALTER vs ha_maria vs App-time Periods.
      
      When a Delete_rows_event (or update) is executed, a lookup handler may be
      created, normally to serve long unique index needs, by a call of
      handler::prepare_for_insert. This function also creates a lookup handler
      if an application-time period exists in a table.
      
      A difference with a usual call of prepare_for_insert is that transactions
      are disabled for this table during ALTER TABLE. See
      mysql_trans_prepare_alter_copy_data call in copy_data_between_tables.
      
      Then, ha_maria calls _ma_tmp_disable_logging_for_table during
      ha_maria::external_lock. It never happened so before, that two handlers
      would be created for write to a single ha_maria table under transactions
      disabled.
      
      Hence, the fix handles this scenario.
      It could be done otherwise, by not creating this lookup handler (since it's
      not used anyway during ONLINE ALTER), but architecturally, two handlers
      should be supported.
      
      Avoiding the creation of lookup handler could be done here additionally,
      but with a cost of slowing down other more generic cases, with an
      additional check of online alter table active.
      6b35d6a9
    • Nikita Malyavin's avatar
      MDEV-30902 Server crash in LEX::first_lists_tables_same · 6f78efc0
      Nikita Malyavin authored
      ONLINE ALTER TABLE uses binlog events like the replication does.
      
      Before it was never used outside of replication, so significant
      change was required. For example, a single event had a statement-like
      befavior: it locked the tables, opened it, and closed them in the end. But
      for ONLINE ALTER we use preopened table.
      
      A crash scenario is following: lex->query_tables was set to NULL in
      restore_empty_query_table_list when alter event is applied.
      Then lex->query_tables->prev_global was write-accessed in
      LEX::first_lists_tables_same, leading to a segfault.
      
      In replication restore_empty_query_table_list would mean resetting lex
      before next query or event.
      
      In ONLINE ALTER TABLE we reuse a locked table between the events, so
      we should avoid it. Here the need to reset lex state (or close the tables)
      can be determined by nonzero rgi->tables_to_lock_count.
      If no table is locked, then event doesn't own the tables.
      
      The same was already done before for rgi->slave_close_thread_tables call.
      6f78efc0
    • Nikita Malyavin's avatar
    • Nikita Malyavin's avatar
      fix main.alter_table_{online,lock} · 84ed2e7c
      Nikita Malyavin authored
      84ed2e7c
    • Nikita Malyavin's avatar
      MDEV-29069 follow-up: improve DEFAULT rules · 41697008
      Nikita Malyavin authored
      previously, fields with DEFAULTs were allowed just when expression is
      deterministic. In case of online alter, we should recursively check that
      underlying fields of expression also either have explicit values, or
      have DEFAULT following this validity rule.
      41697008
    • Nikita Malyavin's avatar
      30756775
    • Nikita Malyavin's avatar
      bac728a2
    • Nikita Malyavin's avatar
      MDEV-29069 ER_KEY_NOT_FOUND on online autoinc addition + concurrent DELETE · 2be4c836
      Nikita Malyavin authored
      We can't rely on keys formed with columns that were added during this
      ALTER. These columns can be set with non-deterministic values, which can
      end up with broken or incorrect search.
      
      The same applies to the keys that contain reliable columns, but also have
      bogus ones. Using them can narrow the search, but they're also ignored.
      
      Also, added columns shouldn't be considered during the record match. To
      determine them, table->has_value_set bitmap is used.
      
      To fill has_value_set bitmap in the find_key call, extra unpack_row call
      has been added.
      
      For replication case, extra replica columns can be considered for this
      case. We try to ignore them, too.
      2be4c836
    • Sergei Golubchik's avatar
    • Sergei Golubchik's avatar
      6ecc90ae
    • Sergei Golubchik's avatar
      cleanup: remove rpl_group_info::get_table_data() · e72ed758
      Sergei Golubchik authored
      use table->pos_in_table_list instead.
      
      Also, table->in_use is always set
      e72ed758
    • Sergei Golubchik's avatar
      cleanup: ifdefs · 8a8f71b8
      Sergei Golubchik authored
      8a8f71b8
    • Nikita Malyavin's avatar
    • Nikita Malyavin's avatar
      daebec60
    • Nikita Malyavin's avatar
      MDEV-29038 XA assertions failing in binlog_rollback and binlog_commit · 754c8dab
      Nikita Malyavin authored
      ONLINE ALTER TABLE adds binlog handlerton into ha_list, so any
      rollback command can end up calling binlog_rollback having no cache_mngr,
      if binlog is not enabled.
      
      The assertion should be fixed in the same manner as DBUG_ASSERT(WSREP(thd))
      754c8dab
    • Nikita Malyavin's avatar
      log_event.h: remove junk EOL spaces · 45bafdbe
      Nikita Malyavin authored
      45bafdbe
    • Nikita Malyavin's avatar
      MDEV-29013 ER_KEY_NOT_FOUND/lock timeout upon online alter with long unique · 6e0f4560
      Nikita Malyavin authored
      1. ER_KEY_NOT_FOUND
      general replcation problem, already fixed earlier.
      test added.
      
      2. ER_LOCK_WAIT_TIMEOUT
      This is a long unique specific problem.
      
      Sometimes, lookup_handler is created for to->file. To properly free it,
      ha_reset should be called. It is usually done by calling
      close_thread_table, but ALTER TABLE makes it differently. Hence, a single
      ha_reset call is added to mysql_alter_table.
      
      Also, event_mem_root is removed. Normally, no per-event data should be
      allocated on thd->mem_root, that would mean a leak. And otherwise,
      lookup_handler is lazily allocated, but its lifetime matches statement,
      not event.
      6e0f4560
    • Sergei Golubchik's avatar
      Fix write_set too · d6e0d29f
      Sergei Golubchik authored
      * in online ALTER it must include the complete new row,
        note that an UPDATE should set all extra columns to their
        default values, as if UPDATE was completely done before the ALTER.
      * in rpl WRITE_ROWS_EVENT it must include all extra slave columns,
        but not existing columns unmarked in the m_cols (sequences do that)
      * in rpl UPDATE/DELETE events it should follow m_cols_ai
      
      also: default values must be updated for WRITE_ROWS_EVENT and
      for UPDATE/DELETE in the online ALTER mode, see above.
      Update the result file accordingly.
      
      Extend bitmap_copy() to support arguments of different lengths
      d6e0d29f
    • Sergei Golubchik's avatar
      MDEV-28816 Assertion `wsrep_thd_is_applying(thd)' failed in int... · 4a00bc87
      Sergei Golubchik authored
      MDEV-28816 Assertion `wsrep_thd_is_applying(thd)' failed in int wsrep_ignored_error_code(Log_event*, int)
      
      wsrep expected that any Rows_log_event::do_apply_event() failures
      can only happen in the applier thread, because no other thread ever
      applies row events. With online alter it's no longer true.
      4a00bc87
    • Nikita Malyavin's avatar
      da5df339
    • Sergei Golubchik's avatar
    • Sergei Golubchik's avatar
      don't do ALTER IGNORE TABLE online · 472c3d08
      Sergei Golubchik authored
      because online means we'll apply events from the binlog, and
      ignore means that bad rows will be skipped. So a bad Write_row_log_event
      will be skipped and a following Update_row_log_event will fail to
      apply.
      472c3d08
    • Nikita Malyavin's avatar
      MDEV-29021 add test case from MDEV-29013 · aa9e173e
      Nikita Malyavin authored
      This test case was also fixed by adding update_virtual_columns.
      aa9e173e
    • Nikita Malyavin's avatar
      Do not ignore sql_mode when replicating · b0db7239
      Nikita Malyavin authored
      Division by zero is a good example. sql_mode is basically ignored by
      replication, see Bug#56662.
      
      The behavior for ONLINE should remain the same as for non-ONLINE ALTER.
      b0db7239
    • Nikita Malyavin's avatar
      Simplify rgi->get_table_data call · bdbd3573
      Nikita Malyavin authored
      bdbd3573
    • Nikita Malyavin's avatar
      b8c5f94b
    • Sergei Golubchik's avatar
      MDEV-29021 mark fields that have explicit values · a3d1d148
      Sergei Golubchik authored
      so that table->update_default_fields() would know what to update
      a3d1d148
    • Nikita Malyavin's avatar
      MDEV-29021 ALTER TABLE fails when a stored virtual column is dropped+added · 93fb92d3
      Nikita Malyavin authored
      We shouldn't rely on `fill_extra_persistent_columns`, as it only updates
      fields which have an index > cols->n_bits (replication bitmap width).
      Actually, it should never be used, as its approach is error-prone.
      
      Normal update_virtual_fields+update_default_fields should be done.
      93fb92d3
    • Sergei Golubchik's avatar
      cleanup, remove dead code · ea46fdce
      Sergei Golubchik authored
      ea46fdce
    • Sergei Golubchik's avatar
      MDEV-28943 Online alter fails under LOCK TABLE with ER_ALTER_OPERATION_NOT_SUPPORTED_REASON · 845c9396
      Sergei Golubchik authored
      if ALTER TABLE ... LOCK=xxx is executed under LOCK TABLES,
      ignore the LOCK clause, because ALTER should not downgrade
      already taken EXCLUSIVE table lock to SHARED or NONE.
      
      This commit preserves the existing behavior (LOCK was de facto ignored),
      but makes it explicit.
      845c9396
    • Nikita Malyavin's avatar
      MDEV-28930 ALTER TABLE Deadlocks with parallel TL_WRITE · 2ed03a41
      Nikita Malyavin authored
      ALTER ONLINE TABLE acquires table with TL_READ. Myisam normally acquires
      TL_WRITE for DML, which makes it hang until table is freed.
      
      We deadlock once ALTER upgrades its MDL lock.
      
      Solution:
      Unlock table earlier. We don't need to hold TL_READ once we finished
      copying. Relay log replication requires no data locks on `from` table.
      2ed03a41
    • Sergei Golubchik's avatar
      MDEV-28967 Assertion `marked_for_write_or_computed()' failed in... · 8fbdc760
      Sergei Golubchik authored
      MDEV-28967 Assertion `marked_for_write_or_computed()' failed in Field_new_decimal::store_value / online_alter_read_from_binlog`
      
      in the catch-up phase of the online alter we apply row events,
      they're unpacked into `from->record[0]` and then converted
      to `to->record[0]`.
      
      This needs all fields of `from` to be in the `write_set`.
      
      Although practically `Field::unpack()` does not assert the `write_set`,
      and `Field::reset()` - used when a field value is not present in the
      after-image - also doesn't assert the `write_set` for many types,
      `Field_new_decimal::reset()` does.
      8fbdc760