Hello Sergei, - This patch: https://github.com/MariaDB/server/commit/4f2d0541b501a7a27bf44aa16821d093951... cleanup: remove innodb-specific code around update_auto_increment() is OK to push. Please write in the commit comments that it was a dead code and that InnoDB team does know why this code was there. - This patch https://github.com/MariaDB/server/commit/c8d0f3370b7d77ecb07f8f1357da6bb1f0d... MDEV-32839 LONG UNIQUE gives error when used with REPLACE is OK. I have only a proposal about tests. You added tests above the "# End of 10.5 tests" line, but the patch is for 10.6. Also please add tests for at lease these engines: - MyISAM - InnoDB - Spider as well as partitioned tables for the same engines, because your patch for ha_partition.cc changes the code related to need_info_for_auto_inc(), which has a special implementation in ha_partition and ha_spider. - This patch: https://github.com/MariaDB/server/commit/94f26086663b7c8dbcd6b26930b64de36a7... cleanup: remove partition-specific code around update_auto_increment()
diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc index 25bbcfdd75ce0..e174ef8d21940 100644 --- a/sql/ha_partition.cc +++ b/sql/ha_partition.cc @@ -4308,6 +4308,7 @@ int ha_partition::external_lock(THD *thd, int lock_type) m_part_info->part_expr->walk(&Item::register_field_in_read_map, 1, 0); if ((error= m_part_info->vers_set_hist_part(thd))) goto err_handler; + need_info_for_auto_inc();
auto-increment code is often done under a condition like this: if (table->next_number_field && buf == table->record[0]) { some_auto_increment code; } This includes the code that you are removing from ha_partition::write_row() Is it ok to call need_info_for_auto_inc() unconditionally here?
} DBUG_RETURN(0);
@@ -4623,33 +4624,8 @@ int ha_partition::write_row(const uchar * buf) */ if (have_auto_increment) { - if (!table_share->next_number_keypart) - if (unlikely(error= update_next_auto_inc_val())) - goto exit;
Here there was an extra condition !table_share->next_number_keypart before calling update_next_auto_inc_val(). Why was it needed? Why it's not needed in the new version of external_lock() before the call for need_info_for_auto_inc()? Also, the old code checked the return value. The new code int external_code() does not check the result of need_info_for_auto_inc(). Can't the behavior change because of this change?
- - /* - If we have failed to set the auto-increment value for this row, - it is highly likely that we will not be able to insert it into - the correct partition. We must check and fail if necessary. - */ if (unlikely(error= update_auto_increment())) goto exit; - - /* - Don't allow generation of auto_increment value the partitions handler. - If a partitions handler would change the value, then it might not - match the partition any longer. - This can occur if 'SET INSERT_ID = 0; INSERT (NULL)', - So allow this by adding 'MODE_NO_AUTO_VALUE_ON_ZERO' to sql_mode. - The partitions handler::next_insert_id must always be 0. Otherwise - we need to forward release_auto_increment, or reset it for all - partitions. - */ - if (table->next_number_field->val_int() == 0) - { - table->auto_increment_field_not_null= TRUE; - thd->variables.sql_mode|= MODE_NO_AUTO_VALUE_ON_ZERO; - }
Why is it safe to remove this code?
} old_map= dbug_tmp_use_all_columns(table, &table->read_set); error= m_part_info->get_partition_id(m_part_info, &part_id, &func_value); @@ -11012,10 +10988,7 @@ void ha_partition::get_auto_increment(ulonglong offset, ulonglong increment, else { THD *thd= ha_thd(); - /* - This is initialized in the beginning of the first write_row call. - */ - DBUG_ASSERT(part_share->auto_inc_initialized); + update_next_auto_inc_val(); /* Get a lock for handling the auto_increment in part_share for avoiding two concurrent statements getting the same number. diff --git a/sql/ha_partition.h b/sql/ha_partition.h index d75e38ec272a5..0ac8f696fcf55 100644 --- a/sql/ha_partition.h +++ b/sql/ha_partition.h @@ -1408,9 +1408,8 @@ class ha_partition final :public handler { ulonglong nr= (((Field_num*) field)->unsigned_flag || field->val_int() > 0) ? field->val_int() : 0; + update_next_auto_inc_val(); lock_auto_increment(); - DBUG_ASSERT(part_share->auto_inc_initialized || - !can_use_for_auto_inc_init()); /* must check when the mutex is taken */ if (nr >= part_share->next_auto_inc_val) part_share->next_auto_inc_val= nr + 1;