Hi Sergei, On Sat, Apr 10, 2021 at 11:12 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Apr 09, Aleksey Midenkov wrote:
--echo # Increment from 3 to 5 --echo # Increment from 3 to 6, manual names, LOCK TABLES --echo # Multiple increments in single command
Besides "increment" is correct because PARTITIONS number is incremented.
Sure. "Increment the number of partitions", this is fine. "Auto create partitions" is also fine. "Increment partitions" is meaningless.
It is obvious from the context that we are talking about the number, not partitions themselves. Treat "partitions" as PARTITIONS keyword and the increment is attached to a number right next to it. That's quite a sense, isn't it?
No, I think it's a meaningless combination of words. But let's ask native speakers, shall we?
The ability to imply things and make special terms is the freedom of any language I suppose. Slang (or "special language") helps us to describe technical entities more concisely.
+--echo # Here t2 is incremented too, but not updated +set timestamp= unix_timestamp('2000-01-01 03:00:00'); +update t1, t2 set t1.x= 0 where t1.x< t2.y; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +# Multiupdate_prelocking_strategy::handle_end() is processed after table open. +# For PS it is possible to skip unneeded auto-creation because the above happens at +# prepare stage and auto-creation is done at execute stage. +--replace_result $default_engine DEFAULT_ENGINE 'PARTITIONS 4' 'PARTITIONS ok' 'PARTITIONS 5' 'PARTITIONS ok'
eh... I don't think this is really "ok". As far as I remember, Multiupdate_prelocking_strategy knows what tables should be opened for writing and what for reading. Why would a new partition be created for t2?
It knows this after tables are opened. Look at handle_end(), specifically mysql_handle_derived(), handle_derived(), setup_fields_with_no_wrap() and check_fields(). I believe all these calls are required to get proper get_table_map().
To get this working properly there must be 2-staged open tables, something like PS does.
You mean, a new partition is created for t2 in normal mode and not created in --ps?
Yes.
What if you'll also add `&& table_list->updating` to your condition in TABLE::vers_need_hist_part() ?
If `Multiupdate_prelocking_strategy::handle_end()` was not yet executed how `updating` can help? No, it doesn't help.
Okay. I think I know how to fix it, but let's do it after the main thing is pushed.
Users are rather sensitive to cases when a non-updatable table in multi-update gets treated like updatable, we've had a bunch of bug reports about it over the years. Wrong privileges (UPDATE privilege should not be checked), wrong locks (the table should never be write-locked, a concurrent write lock should not block multi-update), triggers (should not be opened, so any problems with triggers, like privileges or missing tables, should not affect multi-update), and so on.
So I suppose we'll have to fix this one too. This is how it could be done - vers_need_hist_part should check for table_list->updating. It's always FALSE in multi-update, so it won't be auto-creating new partitions at all for multi-update. But! after handle_end() (or, better, in handle_end()) we'll have another pass over tables and will implement a fallback-and-retry from handle_end().
Let's do it as a separate commit after the feature is pushed.
@@ -2032,6 +2083,20 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx) tc_add_table(thd, table); }
+ if (!ot_ctx->vers_create_count &&
what does this condition mean? When is ot_ctx->vers_create_count > 0 ?
When we already requested backoff action.
I'd suggest to rewrite as
if (ot_ctx->vers_create_count) /* already tried to add a partition to this table and failed (because of e.g. lock conflict). Don't try again */ else if (table->vers_need_hist_part(thd, table_list)) ... and here your code ...
The comment is wrong. Added correct comment.
-void partition_info::vers_set_hist_part(THD *thd) +uint partition_info::vers_set_hist_part(THD *thd, bool auto_hist) { + DBUG_ASSERT(!thd->lex->last_table() || + !thd->lex->last_table()->vers_conditions.delete_history);
is that right? Conceptually you need to test vers_conditions.delete_history for the table that owns this partition_info. Is it always last_table() ?
I'd say it'd be more logical to do, like
part_field_array[0]->table->pos_in_table_list
Sorry, I had to revert that back. The original form was selected not by coincidence. This data in TABLE is not yet initialized. I also have to revert DBUG_ASSERT() you suggested which fails drop_table_force on winx64-debug and add error code condition instead: @@ -3268,12 +3271,12 @@ Open_table_context::recover_from_failed_open() case OT_DISCOVER: case OT_REPAIR: case OT_ADD_HISTORY_PARTITION: - DBUG_ASSERT(!m_thd->get_stmt_da()->is_set()); result= lock_table_names(m_thd, m_thd->lex->create_info, m_failed_table, NULL, get_timeout(), 0); if (result) { - if (m_action == OT_ADD_HISTORY_PARTITION) + if (m_action == OT_ADD_HISTORY_PARTITION && + m_thd->get_stmt_da()->sql_errno() == ER_LOCK_WAIT_TIMEOUT) { // MDEV-23642 Locking timeout caused by auto-creation affects original DML m_thd->clear_error();
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok