Sergei, On Wed, Jun 2, 2021 at 2:00 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jun 02, Aleksey Midenkov wrote:
diff --git a/mysql-test/suite/versioning/t/partition.test b/mysql-test/suite/versioning/t/partition.test --- a/mysql-test/suite/versioning/t/partition.test +++ b/mysql-test/suite/versioning/t/partition.test @@ -1068,13 +1078,456 @@ alter table t1 add partition partitions 2; --replace_result $default_engine DEFAULT_ENGINE show create table t1; alter table t1 add partition partitions 8; +drop table t1; + +--echo # +--echo # End of 10.5 tests +--echo # + +--echo # +--echo # MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT +--echo # +create or replace table t1 (x int) with system versioning +partition by system_time limit 1 auto; +--replace_result $default_engine DEFAULT_ENGINE +show create table t1; +
in the previous version of this patch you had clarifying comments here, like
--echo # INSERT, INSERT .. SELECT don't auto-create partitions
or
--echo # Number of partitions goes from 3 to 5
I kind of miss them now, they were helpful
Yes, they were helpful as is. I removed them like I promised in the previous thread. Though, Sergei, you are free to add your own comments in the form you like. I'm really sorry, but nobody won't tell me what words to use unless I am not able to be clear and bear the common sense.
okay
What about my suggested comments above? Look clear enough?
Like I said, you are free to add your own comments in the form you like. I'd prefer not to interfere with that process.
diff --git a/sql/ha_partition.h b/sql/ha_partition.h --- a/sql/ha_partition.h +++ b/sql/ha_partition.h @@ -1607,7 +1607,7 @@ class ha_partition :public handler for (; part_id < part_id_end; ++part_id) { handler *file= m_file[part_id]; - DBUG_ASSERT(bitmap_is_set(&(m_part_info->read_partitions), part_id)); + bitmap_set_bit(&(m_part_info->read_partitions), part_id);
would be good to have a comment before the method that it's only used in vers_set_hist_part(), if (vers_info->limit).
because if it's a generic method to return the number of rows in a partition (incl. subpartitions) then it's totally not clear why it modifies read_partitions bitmap.
or may be you'd want to rename the method to make it look more specific for LIMIT partitions in vers_set_hist_part() ?
Added a comment if it will matter to anyone. No, I prefer generic and easy names instead of long and capacious ones. I believe the latter ones are non-productive.
I also prefer generic names, but only if the method is generic.
I feel that modifying the read_partitions bitmap as a side effect makes this function rather specialized.
Generally there are a lot of complications and exceptions everywhere in the world. That is the nature of life. Or that is just a limitation of us to not see the higher level of order in the observed entropy. If we always mention and signify that, we'll spend too much energy in vain. That is called tediousness. But I made a good enough comment for the whole function, so I hope that will satisfy both of us.
ok, let's try that.
generally, as far as a big code base is concerned, I'd rather prefer tediousness over the excitement of multi-hour debugging and discovering an unexpected side effect of a generic function.
Did that ever helped? Not for me.
diff --git a/sql/lock.cc b/sql/lock.cc --- a/sql/lock.cc +++ b/sql/lock.cc @@ -662,16 +662,28 @@ MYSQL_LOCK *mysql_lock_merge(MYSQL_LOCK *a,MYSQL_LOCK *b) DBUG_PRINT("enter", ("a->lock_count: %u b->lock_count: %u", a->lock_count, b->lock_count));
- if (!(sql_lock= (MYSQL_LOCK*) - my_malloc(key_memory_MYSQL_LOCK, sizeof(*sql_lock) + - sizeof(THR_LOCK_DATA*)*((a->lock_count+b->lock_count)*2) + - sizeof(TABLE*)*(a->table_count+b->table_count),MYF(MY_WME)))) - DBUG_RETURN(0); // Fatal error + const size_t lock_size= sizeof(*sql_lock) + + sizeof(THR_LOCK_DATA *) * ((a->lock_count + b->lock_count) * 2) + + sizeof(TABLE *) * (a->table_count + b->table_count); + if (thd) + { + sql_lock= (MYSQL_LOCK *) thd->alloc(lock_size); + if (!sql_lock) + DBUG_RETURN(0); + sql_lock->flags= GET_LOCK_ON_THD;
This doesn't answer my question. Was it part of the MDEV-23639 bug fix? It was part of the MDEV-23639 commit.
Sorry, I meant the moment when MDEV-23639 calls mysql_lock_merge(). Yes, that is the part of the bug fix.
What did it fix and how?
I can understand how changing some memory allocation from MEM_ROOT to malloc could fix a bug (if MEM_ROOT lifetime is too short for the object), but changing from malloc to MEM_ROOT? This could be a good optimization, but how can it fix anything?
That's not about changing the method, that's about merging locks. When I merged my locks with thd there were already thd-allocated locks.
+ /* + NOTE: The semantics of vers_set_hist_part() is double: even when we
twofold
Dual, double or twofold. What is the difference between the synonyms?
https://diffsense.com/diff/double/twofold
here it's used as an adjective. The page above explains:
Twofold: having two parts. Examples: "a twofold nature; a twofold sense; a twofold argument"
This site says:
When used as adjectives, ... twofold means double.
Do you really believe in that butter butterish? :) I mean do we need to discuss all sorts of butter? That level of white noise should be ignored.
I'm just trying to avoid incorrect usage of a language.
I wish that threshold of incorrectness would be more relaxed.
+ don't need auto-create, we need to update part_info->hist_part. + */ ... + else + { + /* + NOTE: this may repeat multiple times until creating thread acquires + MDL_EXCLUSIVE. Since auto-creation is rare operation this is acceptable. + We could suspend this thread on cond-var but we must first exit + MDL_SHARED_WRITE first and we cannot store cond-var into TABLE_SHARE + because it is already released and there is no guarantee that it will + be same instance if we acquire it again. + */ + table_list->vers_skip_create= false; + ot_ctx->vers_create_count= 0; + action= Open_table_context::OT_REOPEN_TABLES; + table_arg= NULL; + }
I'm afraid I don't understand. All this business with vers_skip_create and vers_skip_auto_create, it wasn't in the previous version of the patch. So, I believe it was a fix for one of the MDEV bugs reported and fixed meanwhile.
What MDEV was it? Could you explain what was the issue and what was the fix, please?
No, that was the multi-threaded case which worked good for me, but suddenly I discovered it fails on some buildbot.
Could you elaborate on what the problem was? Two threads trying to add the partition in parallel? I'd expect MDL_EXCLUSIVE to prevent that.
MDL_EXCLUSIVE prevents parallel execution of repair_from_failed_open(), but not sequential. So it can add several partitions instead of 1, one after another.
What's the sequence of events? One thread decides to add a partition, takes an MDL_EXCLUSIVE, the other thread decides to add a partition, waits for MDL_EXCLUSIVE, the first one finishes adding a partition, releases the lock, the second grabs it and adds a second partition?
Right.
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok