review for MDEV-32839 LONG UNIQUE gives error when used with REPLACE
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;
Hi, Alexander, On Dec 06, Alexander Barkov wrote:
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.
ok
- 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.
because I wanted to cherry-pick it into 10.5 :) one conflict less if I add the test where it'll be
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.
ok
- 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?
I think so. It only sets `auto_inc_initialized= FALSE`. If `table->next_number_field` is false, it won't do anything anyway, and `buf == table->record[0]` doesn't apply here, because this is external_lock, it's done once, not executed per row.
} 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?
It's to detect the case when auto-inc is the first part of the key, so it's always incremented, the value is (probably) stored in the engine, etc. When auto-inc is not the first key part, it's reset for every new group of rows and this is a very different behavior.
Why it's not needed in the new version of external_lock() before the call for need_info_for_auto_inc()?
Because need_info_for_auto_inc() only sets `auto_inc_initialized= FALSE`. `update_next_auto_inc_val()` is now invoked from `ha_partition::get_auto_increment()` and it's there under the same `!table_share->next_number_keypart` condition.
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?
No, `need_info_for_auto_inc()` does't return anything useful there, it's only purpose is to set `auto_inc_initialized`.
- - /* - 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?
Because it doesn't seem to be doing anything. There's another change in `ha_partition::update_row()` that does Field *saved_next_number_field= table->next_number_field; table->next_number_field= NULL; error= m_file[new_part_id]->ha_write_row((uchar*) new_data); table->next_number_field= saved_next_number_field; So, `table->next_number_field` is always NULL for underlying engines. Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
Hello Sergei. Thanks for your comments. I have no other questions. Ok to push after fixing the issues that we agreed on below. On 12/12/23 16:52, Sergei Golubchik wrote:
Hi, Alexander,
On Dec 06, Alexander Barkov wrote:
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.
ok
- 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.
because I wanted to cherry-pick it into 10.5 :) one conflict less if I add the test where it'll be
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.
ok
- 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?
I think so. It only sets `auto_inc_initialized= FALSE`.
If `table->next_number_field` is false, it won't do anything anyway, and `buf == table->record[0]` doesn't apply here, because this is external_lock, it's done once, not executed per row.
} 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?
It's to detect the case when auto-inc is the first part of the key, so it's always incremented, the value is (probably) stored in the engine, etc.
When auto-inc is not the first key part, it's reset for every new group of rows and this is a very different behavior.
Why it's not needed in the new version of external_lock() before the call for need_info_for_auto_inc()?
Because need_info_for_auto_inc() only sets `auto_inc_initialized= FALSE`. `update_next_auto_inc_val()` is now invoked from `ha_partition::get_auto_increment()` and it's there under the same `!table_share->next_number_keypart` condition.
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?
No, `need_info_for_auto_inc()` does't return anything useful there, it's only purpose is to set `auto_inc_initialized`.
- - /* - 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?
Because it doesn't seem to be doing anything. There's another change in `ha_partition::update_row()` that does
Field *saved_next_number_field= table->next_number_field; table->next_number_field= NULL; error= m_file[new_part_id]->ha_write_row((uchar*) new_data); table->next_number_field= saved_next_number_field;
So, `table->next_number_field` is always NULL for underlying engines.
Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org
participants (2)
-
Alexander Barkov -
Sergei Golubchik