Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Wed, Jun 2, 2021 at 11:37 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Jun 02, Aleksey Midenkov wrote:
> > On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik wrote:
> > >
> > > On Jun 02, Aleksey Midenkov wrote:
> > > > > > > > > >
> > > > > > > > > > - 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;
> > > > > > > > >
> > > > > > 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?
> > > > >
> > > > 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.
> > >
> > > And? How did allocating locks on THD fix anything?
> >
> > LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
> > enough for you?
>
> I'm just trying to understand what the bug was. And I still cannot.
This was not a bug, this was a new change in recover_from_failed_open().
>
> > Also freeing was impossible for locks on thd.
>
> Yes, this change I understand, no questions about freeing.
>
> > > > > > > > > > + /*
> > > > > > > > > > + NOTE: The semantics of vers_set_hist_part() is
> > > > > > > > > > double:
> > > > > > > > >
> > > > > > > > > twofold
>
> Please, fix the language to be proper English.
Don't you want to ask Ian now? Please look for "double semantics"
collocation in Google query (quotes are important). There are quite a
number of examples including scientific books and IETF drafts.
>
> > > > > > > > > > + 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.
> > > > > > > >
> > > > > > > > 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.
> > >
> > > Okay. Then, why a thread didn't check the number of partitions after
> > > acquiring MDL_EXCLUSIVE? Like with mutexes, if there's a mutex that
> > > protects a shared variable, you first acquire the mutex, then read
> > > the variable's value. Not the other way around.
> >
> > Number of partitions is not a shared variable. part_info is kept in
> > TABLE instance. To get new value it must reacquire share, reparse
> > part_sql string. Then comparing with what? After releasing
> > MDL_SHARED_WRITE everything is gone: TABLE, TABLE_SHARE. You must
> > store somewhere old value, presumably in Open_table_context.
>
> I thought that after acquiring MDL_EXCLUSIVE, just as the thread is
> trying to add a new partition, it could check the conditions if the new
> partition, indeed, needs to be added.
If it were so easy as it sounds I'd make it.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
@midenok
Re: [Maria-developers] 557ab2992dd: MDEV-21117 Incremental patch for a few review notes addressed.
Hi, Andrei!
Again, this is a review of the git diff f09d33f521b 557ab2992dd,
not only 557ab2992dd.
On Jun 02, Andrei Elkin wrote:
> revision-id: 557ab2992dd (mariadb-10.6.0-67-g557ab2992dd)
> parent(s): 4cc5a3074da
> author: Andrei Elkin
> committer: Andrei Elkin
> timestamp: 2021-05-27 17:37:22 +0300
> message:
>
> MDEV-21117 Incremental patch for a few review notes addressed.
> diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc
> b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc
> new file mode 100644
> --- /dev/null
> +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.inc
> @@ -0,0 +1,65 @@
> +connect(master1,localhost,root,,);
> +connect(master2,localhost,root,,);
> +connect(master3,localhost,root,,);
> +
> +--connection default
> +
> +# First to commit few transactions
> +INSERT INTO t VALUES (10);
> +INSERT INTO tm VALUES (10);
> +
> +--connection master1
> +# Hold insert after write to binlog and before "run_commit_ordered" in engine
> +SET DEBUG_SYNC= "commit_before_get_LOCK_commit_ordered SIGNAL master1_ready
> WAIT_FOR signal_never_arrives";
> +--send_eval $query1
> +
> +--connection master2
> +SET DEBUG_SYNC= "now WAIT_FOR master1_ready";
> +SET DEBUG_SYNC= "commit_before_get_LOCK_after_binlog_sync SIGNAL
> master2_ready";
> +if ($pre_q2)
> +{
> + --send_eval $pre_q2;
> +}
> +# To binlog non-xid transactional group which will be truncated all right
> +if (!$pre_q2)
> +{
> + --send_eval $query2
> +}
eh, this makes no sense. In the previous version of the patch
you've run pre_q2 before sending query2.
now you send either pre_q2 or query2.
so you don't need pre_q2 anymore now, just set query2 to the
what used to be pre_q2.
> +
> +--connection master3
> +SET DEBUG_SYNC= "now WAIT_FOR master2_ready";
> +SELECT @@global.gtid_binlog_pos as 'Before the crash';
> +
> +--connection default
> +--source include/kill_mysqld.inc
> +--disconnect master1
> +--disconnect master2
> +--disconnect master3
> +
> +#
> +# Server restart
> +#
> +--let $restart_parameters= --rpl-semi-sync-slave-enabled=1
> +--source include/start_mysqld.inc
> +
> +# Check error log for a successful truncate message.
> +--let $log_error_ = $MYSQLTEST_VARDIR/log/mysqld.1.err
> +
> +--let SEARCH_FILE=$log_error_
> +--let SEARCH_PATTERN=Successfully truncated.*to remove transactions starting
> from GTID $truncate_gtid_pos
> +--replace_regex /FOUND [0-9]+/FOUND #/
can it be found an unpredictable number of times?
Why would binlog be truncated more than once?
> +--source include/search_pattern_in_file.inc
> +
> +--echo Pre-crash binlog file content:
> +--let $binlog_file= query_get_value(show binary logs, Log_name,
> $binlog_file_index)
> +--source include/show_binlog_events.inc
> +
> +SELECT @@global.gtid_binlog_pos as 'After the crash';
> +--echo "One row should be present in table 't'"
> +SELECT * FROM t;
> +
> +# prepare binlog file index for the next test
> +--inc $binlog_file_index
> +
> +# Local cleanup
> +DELETE FROM t;
> diff --git a/mysql-test/suite/binlog/t/binlog_truncate_active_log.test
> b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test
> new file mode 100644
> --- /dev/null
> +++ b/mysql-test/suite/binlog/t/binlog_truncate_active_log.test
> @@ -0,0 +1,104 @@
> +# Purpose
> +#
> +# Test verifies the truncation of single binary log file.
> +#
> +# References
> +#
> +# MDEV-21117: recovery for --rpl-semi-sync-slave-enabled server
> +
> +--source include/have_innodb.inc
> +--source include/have_aria.inc
> +# File: binlog_truncate_active_log.inc included in test makes use of
> +# 'debug_sync' facility.
> +--source include/have_debug_sync.inc
> +--source include/have_binlog_format_statement.inc
> +
> +call mtr.add_suppression("Can.t init tc log");
> +call mtr.add_suppression("Aborting");
> +
> +# The following cases are tested:
> +# A. 2pc transaction is followed by a blank "zero-engines" one
> +# B. 2pc transaction follows the blank one
> +# C. Similarly to A, with the XA blank transaction
> +
> +RESET MASTER;
> +CREATE TABLE t (f INT) ENGINE=INNODB;
> +CREATE TABLE t2 (f INT) ENGINE=INNODB;
> +CREATE TABLE tm (f INT) ENGINE=Aria;
> +
> +# Old (pre-crash) binlog file index initial value.
> +# It keeps incremented at the end of each case.
> +--let $binlog_file_index=1
> +
> +--echo # Case A.
> +# Using 'debug_sync' hold 'query1' execution after 'query1' is flushed and
> +# synced to binary log but not yet committed. In an another connection hold
> +# 'query2' execution after 'query2' is flushed and synced to binlog.
> +# Crash and restart server with --rpl-semi-sync-slave-enabled=1
> +#
> +# During recovery of binary log 'query1' status is checked with InnoDB
> engine,
> +# it will be in prepared but not yet commited. All transactions starting from
> +# 'query1' onwards will be removed from the binary log.
> +# Show-binlog-events is to prove that.
> +
> +--let $truncate_gtid_pos = 0-1-6
> +--let $query1 = INSERT INTO t VALUES (20)
> +--let
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Jun 02, Aleksey Midenkov wrote:
> On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik wrote:
> >
> > On Jun 02, Aleksey Midenkov wrote:
> > > > > > > > >
> > > > > > > > > - 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;
> > > > > > > >
> > > > > 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?
> > > >
> > > 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.
> >
> > And? How did allocating locks on THD fix anything?
>
> LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
> enough for you?
I'm just trying to understand what the bug was. And I still cannot.
> Also freeing was impossible for locks on thd.
Yes, this change I understand, no questions about freeing.
> > > > > > > > > + /*
> > > > > > > > > + NOTE: The semantics of vers_set_hist_part() is double:
> > > > > > > >
> > > > > > > > twofold
Please, fix the language to be proper English.
> > > > > > > > > + 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.
> > > > > > >
> > > > > > > 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.
> >
> > Okay. Then, why a thread didn't check the number of partitions after
> > acquiring MDL_EXCLUSIVE? Like with mutexes, if there's a mutex that
> > protects a shared variable, you first acquire the mutex, then read
> > the variable's value. Not the other way around.
>
> Number of partitions is not a shared variable. part_info is kept in
> TABLE instance. To get new value it must reacquire share, reparse
> part_sql string. Then comparing with what? After releasing
> MDL_SHARED_WRITE everything is gone: TABLE, TABLE_SHARE. You must
> store somewhere old value, presumably in Open_table_context.
I thought that after acquiring MDL_EXCLUSIVE, just as the thread is
trying to add a new partition, it could check the conditions if the new
partition, indeed, needs to be added.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Sergei,
On Wed, Jun 2, 2021 at 9:06 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Jun 02, Aleksey Midenkov wrote:
> > > > > > > >
> > > > > > > > - 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;
> > > > > > >
> > > > 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?
> > >
> > 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.
>
> And? How did allocating locks on THD fix anything?
LTM_PRELOCKED had locks on thd, I keep merged locks on thd. Isn't that
enough for you? Also freeing was impossible for locks on thd.
>
> > > > > > > > + /*
> > > > > > > > + NOTE: The semantics of vers_set_hist_part() is double:
> > > > > > >
> > > > > > > twofold
> > > > > >
> > > > 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.
>
> Sure. If any authoritative source will show that both words are correct
> in this context, I'll readily admit that I'm wrong and will remember it
> for the future. I didn't comment on a whim, it wasn't a matter of taste,
> it was as far as I know an actual incorrect usage of words, an error.
Go ahead and play that game 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.
> > > > > >
> > > > > > 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.
>
> Okay. Then, why a thread didn't check the number of partitions after
> acquiring MDL_EXCLUSIVE? Like with mutexes, if there's a mutex that
> protects a shared variable, you first acquire the mutex, then read the
> variable's value. Not the other way around.
Number of partitions is not a shared variable. part_info is kept in
TABLE instance. To get new value it must reacquire share, reparse
part_sql string. Then comparing with what? After releasing
MDL_SHARED_WRITE everything is gone: TABLE, TABLE_SHARE. You must
store somewhere old value, presumably in Open_table_context.
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and secur...@mariadb.org
--
All the best,
Aleksey Midenkov
@midenok
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey!
On Jun 02, Aleksey Midenkov wrote:
> > > > > > >
> > > > > > > - 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;
> > > > > >
> > > 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?
> >
> 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.
And? How did allocating locks on THD fix anything?
> > > > > > > + /*
> > > > > > > + NOTE: The semantics of vers_set_hist_part() is double:
> > > > > >
> > > > > > twofold
> > > > >
> > > 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.
Sure. If any authoritative source will show that both words are correct
in this context, I'll readily admit that I'm wrong and will remember it
for the future. I didn't comment on a whim, it wasn't a matter of taste,
it was as far as I know an actual incorrect usage of words, an error.
> > > > > > > + 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.
> > > > >
> > > > > 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.
Okay. Then, why a thread didn't check the number of partitions after
acquiring MDL_EXCLUSIVE? Like with mutexes, if there's a mutex that
protects a shared variable, you first acquire the mutex, then read the
variable's value. Not the other way around.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
[Maria-developers] Tips on Writing/Reading JSON
Hi Alexey, I'm Michael Okoko, a GSoC '21 student working on Using JSON as the on-disk format for MariaDB histograms(https://jira.mariadb.org/browse/MDEV-21130). The project requires writing and reading/parsing JSON, and I was wondering if you could provide some tips and pointers regarding a proper JSON writer/JSON parser to use. Regards, Michael. ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 8e79d168614: MDEV-25672 table alias from previous statement interferes later commands.
Hi Sergei! I'm sorry, what other I can say apart from commits descriptions? This code fixes the roots of problems while you suggest to keep bugs masked. On Wed, Jun 2, 2021 at 3:23 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Jun 02, Aleksey Midenkov wrote: > > revision-id: 8e79d168614 (mariadb-10.2.31-989-g8e79d168614) > > parent(s): 433dd490d33 > > author: Aleksey Midenkov > > committer: Aleksey Midenkov > > timestamp: 2021-05-31 16:20:38 +0300 > > message: > > > > MDEV-25672 table alias from previous statement interferes later commands. > > I'm sorry. I don't understand what all this new code for. > > To make sure that vcol's field->table_name is refreshed for every statement? > What is vcol's field->table_name used for besides your CREATE TABLE check? > > If it's not used for anything then a much simpler fix would be narrow > the check - it only needs to be run for new vcols in CREATE TABLE, > disabling it for old vcols in ALTER TABLE will do the trick. Like > > -if (p.table_name.length && table_name) > +if (!field && p.table_name.length && table_name) > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 8e79d168614: MDEV-25672 table alias from previous statement interferes later commands.
Hi, Aleksey! On Jun 02, Aleksey Midenkov wrote: > revision-id: 8e79d168614 (mariadb-10.2.31-989-g8e79d168614) > parent(s): 433dd490d33 > author: Aleksey Midenkov > committer: Aleksey Midenkov > timestamp: 2021-05-31 16:20:38 +0300 > message: > > MDEV-25672 table alias from previous statement interferes later commands. I'm sorry. I don't understand what all this new code for. To make sure that vcol's field->table_name is refreshed for every statement? What is vcol's field->table_name used for besides your CREATE TABLE check? If it's not used for anything then a much simpler fix would be narrow the check - it only needs to be run for new vcols in CREATE TABLE, disabling it for old vcols in ALTER TABLE will do the trick. Like -if (p.table_name.length && table_name) +if (!field && p.table_name.length && table_name) Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 38a888da0f1: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
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?
> > > > > 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.
> > > > > 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
