- developers - lists.mariadb.org

Re: [MariaDB commits] [PATCH 1/6] MDEV-34696: do_gco_wait() completes too early on InnoDB dict stats updates
by andrei.elkin＠pp.inet.fi 23 Aug '24

23 Aug '24

Howdy Kristian. > Before doing mark_start_commit(), check that there is no pending deadlock > kill. If there is a pending kill, we won't commit (we will abort, roll back, > and retry). Then we should not mark the commit as started, since that could > potentially make the following GCO start too early, before we completed the > commit after the retry. > > This condition could trigger in some corner cases, where InnoDB would take > temporarily table/row locks that are released again immediately, not held > until the transaction commits. This happens with dict_stats updates and > possibly auto-increment locks. > > Such locks can be passed to thd_rpl_deadlock_check() and cause a deadlock > kill to be scheduled in the background. But since the blocking locks are > held only temporarily, they can be released before the background kill > happens. This way, the kill can be delayed until after mark_start_commit() > has been called. Thus we need to check the synchronous indication > rgi->killed_for_retry, not just the asynchroneous thd->killed. I think I understood what's going on, also thanks to a verbose Jira ticket description. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > --- > sql/rpl_parallel.cc | 20 ++++++++++++++++---- > sql/rpl_rli.cc | 17 +++++++++++++++++ > 2 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index 1cfdf96ee3b..9c4222d7817 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -1450,11 +1450,23 @@ handle_rpl_parallel_thread(void *arg) > after mark_start_commit(), we have to unmark, which has at least a > theoretical possibility of leaving a window where it looks like all > transactions in a GCO have started committing, while in fact one > - will need to rollback and retry. This is not supposed to be possible > - (since there is a deadlock, at least one transaction should be > - blocked from reaching commit), but this seems a fragile ensurance, > - and there were historically a number of subtle bugs in this area. > + will need to rollback and retry. > + > + Normally this will not happen, since the kill is there to resolve a > + deadlock that is preventing at least one transaction from proceeding. > + One case it can happen is with InnoDB dict stats update, which can > + temporarily cause transactions to block each other, but locks are > + released immediately, they don't linger until commit. There could be > + other similar cases, there were historically a number of subtle bugs > + in this area. > + > + But once we start the commit, we can expect that no new lock > + conflicts will be introduced. So by handling any lingering deadlock > + kill at this point just before mark_start_commit(), we should be > + robust even towards spurious deadlock kills. > */ > + if (rgi->killed_for_retry != rpl_group_info::RETRY_KILL_NONE) > + wait_for_pending_deadlock_kill(thd, rgi); Assuming my understanding, please correct me if anything, I left a question on this block in https://github.com/MariaDB/server/commit/df0c36a354ffde2766ebed2615642c74b7… quoted further. ... this guard would not let a victim of an optimistic conflict proceed until it clears out itself it is such a victim. For instance in the binlog sequence like T_1, T_2, D_3 where T_2 is the victim, D_3 is a DDL, the victim T_2 won't anymore release the next gco member D_3 into its execution. But what if at this point T_2 is only going to be marked in rgi->killed_for_retry by T_1? That is it is apparently able, with this patch as well, having ...NONE killed status go through. T_1 would reach this very point later and when that is done before T_2 finds itself killed, T_1 would release D_3, and afterward T_2 would finally see itself KILLed into retry. Cheers, Andrei

2 4

Open PRs pending second approver (has already one)
by Otto Kekäläinen 23 Aug '24

23 Aug '24

Hi! There are couple PRs that have 1 approval, and are pending the second one. Would somebody have time to review them? In order of importance: https://github.com/MariaDB/server/pull/3401 "MDEV-34680 Asynchronous and Buffered Logging in MySQL Audit Plugin" https://github.com/MariaDB/server/pull/3406 (related but zero approvals: https://github.com/MariaDB/server/pull/3324 "MDEV-12182 Add client tcp port to audit plugin logs") https://github.com/MariaDB/server/pull/3208 "Refine OpenSSL test names and allow more complete OpenSSL version coverage" https://github.com/MariaDB/server/pull/3225 "MDEV-29955: add ${PCRE_LIBRARY_PATH} to link directories" Thanks, Otto

1 0

Re: 186aa192d1f: MDEV-34704 Quick mode produces the bug for mariadb client
by Sergei Golubchik 22 Aug '24

22 Aug '24

Hi, Sanja, ok to push, but see one comment below On Aug 22, Oleksandr Byelkin wrote: > revision-id: 186aa192d1f (mariadb-11.5.1-34-g186aa192d1f) > parent(s): 9e1923ca238 > author: Oleksandr Byelkin > committer: Oleksandr Byelkin > timestamp: 2024-08-05 16:16:09 +0200 > message: > > MDEV-34704 Quick mode produces the bug for mariadb client > > --quick-max-column-width parameter added to limit field > width in --quick mode. > > diff --git a/mysql-test/main/client.test b/mysql-test/main/client.test > new file mode 100644 > index 00000000000..59e687ff8b9 > --- /dev/null > +++ b/mysql-test/main/client.test > @@ -0,0 +1,33 @@ > +--echo # > +--echo # MDEV-34704: Quick mode produces the bug for mariadb client > +--echo # > + > + > +create table t1 (aaaaaaaaa char (5), aaaaa char (10), a char (127), b char(1)); > +insert into t1 values ("X", "X", "X", "X"); > + to make the result file more readable, I'd suggest to add here, like --echo mysql --table --quick ... --echo mysql --table --quick --quick-max-column-width=0 ... etc before every corresponding --exec > +--exec echo "select * from test.t1;" | $MYSQL --table --quick 2>&1 > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick --quick-max-column-width=0 2>&1 > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick --quick-max-column-width=10 2>&1 > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick --quick-max-column-width=20 2>&1 > + > +insert into t1 values ("01234", "0123456789", "01234567890123456789", "1"); > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick 2>&1 > + > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick --quick-max-column-width=0 2>&1 > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick --quick-max-column-width=10 2>&1 > + > +--exec echo "select * from test.t1;" | $MYSQL --table --quick --quick-max-column-width=20 2>&1 > + > +drop table t1; > + > +--echo # > +--echo # End of 10.7 tests > +--echo # As Marko said, use here only one line --echo # End of 10.7 tests otherwise merge mixes it up with --echo # --echo # MDEV-34704: Quick mode produces the bug for mariadb client --echo # Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

Suggestion: use 'main' as the development branch instead of an ever changing 11.x
by Otto Kekäläinen 22 Aug '24

22 Aug '24

Hi! Has the core developers had discussions about using branch name 'main' for development instead of switching to a new branch every 3 months? If all new features and other additions would always target the branch 'main' it would lower the barrier of entry for new contributors, and decrease the number of unnecessary rebases done by contributors. If the permanent development target was 'main', you would need to create new 11.x branches only at "freeze" when a new major version branch is cut for release. MariaDB is currently the only open source project I know that does *not* have a main/master/trunk branch and instead does these constant new branch names. I imagine this is not only annoying for contributors, but also the core developers who waste time asking contributors to rebase PRs every 3 months (see https://github.com/MariaDB/server/pull/2556 and https://github.com/MariaDB/server/pull/2671 as examples). - Otto

7 11

Re: [MariaDB commits] [PATCH 3/6] Fix sporadic test failure in rpl.rpl_create_drop_event
by Kristian Nielsen 22 Aug '24

22 Aug '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Kristian Nielsen via developers <developers(a)lists.mariadb.org> writes: >> The already-started instance of the event happily continues to run after >> being dropped. I wonder if that's a bug, or if the documentation is >> wrong? > > To this question, I vaguely remember discussions about this matter which > must've concuded in that it's not a bug when the scheduler races ahead > of DROP EVENT to launch a piece of job, like INSERT in this case. Aha, I see. Following up on this, I found this comment in the code Stops the scheduler (again). Waits for acknowledgement from the scheduler that it has stopped - synchronous stopping. Already running events will not be stopped. If the user needs them stopped manual intervention is needed. So then the behaviour seen in the test case is expected, and just fixing the test to handle the possible race should be correct. I also see in the code that the thread running the events starts by loading the event definition from the system tables (mysql.event ...), db_repository->load_named_event(). So there should be no memory issues even if the event is dropped while an instance of it is running. Thanks! - Kristian.

1 0

Re: [MariaDB commits] [PATCH 3/6] Fix sporadic test failure in rpl.rpl_create_drop_event
by Kristian Nielsen 21 Aug '24

21 Aug '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> My question is if it is ok and by design that the event can continue running >> after the scheduler is stopped and even after the event is dropped? That > Should not be ok. The docs https://mariadb.com/kb/en/drop-event/ > > are explicit > > Description > This statement drops the event named event_name. The event immediately ceases being active, and is deleted completely from the server. Thanks for the link, Andrei! Interesting, this doesn't seem to be how it works. This small test starts an event that runs for 3 seconds, but drops the event after 1 second: ----------------------------------------------------------------------- SET GLOBAL event_scheduler= OFF; CREATE TABLE t1(a INT); CREATE EVENT ev1 ON SCHEDULE EVERY 5 SECOND DO INSERT INTO t1 VALUES(SLEEP(3)); SET GLOBAL event_scheduler= ON; --sleep 1 SHOW FULL PROCESSLIST; SET GLOBAL event_scheduler= OFF; DROP EVENT ev1; SHOW FULL PROCESSLIST; --sleep 1 SHOW FULL PROCESSLIST; DROP TABLE t1; ----------------------------------------------------------------------- The already-started instance of the event happily continues to run after being dropped. I wonder if that's a bug, or if the documentation is wrong? And if it is a bug, if there are any use-after-free or similar memory issues with the event continuing to run after being dropped? Valgrind doesn't complain... ----------------------------------------------------------------------- SET GLOBAL event_scheduler= OFF; CREATE TABLE t1(a INT); CREATE EVENT ev1 ON SCHEDULE EVERY 5 SECOND DO INSERT INTO t1 VALUES(SLEEP(3)); Warnings: Warning 1105 Event scheduler is switched off, use SET GLOBAL event_scheduler=ON to enable it. SET GLOBAL event_scheduler= ON; SHOW FULL PROCESSLIST; Id User Host db Command Time State Info Progress 4 root localhost test Query 0 starting SHOW FULL PROCESSLIST 0.000 5 event_scheduler localhost NULL Daemon 1 Waiting for next activation NULL 0.000 6 root localhost test Connect 1 User sleep INSERT INTO t1 VALUES(SLEEP(3)) 0.000 SET GLOBAL event_scheduler= OFF; DROP EVENT ev1; SHOW FULL PROCESSLIST; Id User Host db Command Time State Info Progress 4 root localhost test Query 0 starting SHOW FULL PROCESSLIST 0.000 6 root localhost test Connect 1 User sleep INSERT INTO t1 VALUES(SLEEP(3)) 0.000 SHOW FULL PROCESSLIST; Id User Host db Command Time State Info Progress 4 root localhost test Query 0 starting SHOW FULL PROCESSLIST 0.000 6 root localhost test Connect 2 User sleep INSERT INTO t1 VALUES(SLEEP(3)) 0.000 DROP TABLE t1; ----------------------------------------------------------------------- - Kristian.

1 0

Re: [PATCH 1/6] MDEV-34696: do_gco_wait() completes too early on InnoDB dict stats updates
by Kristian Nielsen 20 Aug '24

20 Aug '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Howdy Kristian. > I was going to review. Thanks Andrei! > Is the patch is any github branch? Looks so by the MDEV. It'd be good to > have it there for reviewing either. Yes, the entire patch series is in bb-10.5-knielsen: https://github.com/MariaDB/server/commits/bb-10.5-knielsen This particular issue is the commit: MDEV-34696: do_gco_wait() completes too early on InnoDB dict stats updates - Kristian.

1 0

Re: [MariaDB commits] [PATCH 3/6] Fix sporadic test failure in rpl.rpl_create_drop_event
by Kristian Nielsen 20 Aug '24

20 Aug '24

Hi Bar, I want to ask you about this fix of a sporadic failure of the test rpl.rpl_create_drop_event, which is authored by you. Not so much the test case fix, which should be good, but I want to ask someone who knows the event scheduler code better than me to be sure I am not merely hiding a real bug with the test case fix. What happens in the test case when it failed is that a new run of an event is started, and this run can continue while the event scheduler is stopped and even after the event itself is dropped: CREATE OR REPLACE EVENT ev1 ON SCHEDULE EVERY 1 SECOND DO INSERT INTO t1 VALUES (11); ... SET GLOBAL event_scheduler=off; DROP EVENT ev1; My question is if it is ok and by design that the event can continue running after the scheduler is stopped and even after the event is dropped? That this doesn't cause any problems like accessing freed memory or such? Or if you do not know, if you know who else I could ask? Thanks, - Kristian. Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > Depending on timing, an extra event run could start just when the event > scheduler is shut down and delay running until after the table has been > dropped; this would cause the test to fail with a "table does not exist" > error in the log. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > --- > mysql-test/suite/rpl/t/rpl_create_drop_event.test | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/mysql-test/suite/rpl/t/rpl_create_drop_event.test b/mysql-test/suite/rpl/t/rpl_create_drop_event.test > index 96a7e82d6f7..79bb0ffec90 100644 > --- a/mysql-test/suite/rpl/t/rpl_create_drop_event.test > +++ b/mysql-test/suite/rpl/t/rpl_create_drop_event.test > @@ -14,6 +14,12 @@ SET GLOBAL event_scheduler=on; > let $wait_condition= SELECT count(*)>0 FROM t1; > --source include/wait_condition.inc > SET GLOBAL event_scheduler=off; > +# If the time rolls to the next whole second just at this point, a new event > +# run may be scheduled. Wait for this to disappear, otherwise we see occasional > +# test failures if the table gets dropped before the extra event run completes. > +# Expect 5 connections: default, master, master1, server_1, binlog dump thread > +--let $wait_condition= SELECT COUNT(*) = 5 FROM INFORMATION_SCHEMA.PROCESSLIST; > +--source include/wait_condition.inc > SELECT DISTINCT a FROM t1; > DELETE FROM t1;

1 0

Re: [PATCH 1/6] MDEV-34696: do_gco_wait() completes too early on InnoDB dict stats updates
by Kristian Nielsen 20 Aug '24

20 Aug '24

Hi Brandon, Andrei, Does one of you want to review this patch for 10.5? It fixes a real (and serious) issue in parallel replication that is seen as sporadic failure of rpl.rpl_start_alter_4 in Buildbot. This is yet another occurrence of the problem where mark_start_commit() is done too early, this code unfortunately is fragile and has had a number of problems in the past. But I am quite confident about the patch, calling wait_for_pending_deadlock_kill() should be very safe, and should be correct here. The added assertion helps detect the problem more reliably and could also help catch any other remaining issues around mark_start_commit (though hopefully there are no more left now). - Kristian. Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > Before doing mark_start_commit(), check that there is no pending deadlock > kill. If there is a pending kill, we won't commit (we will abort, roll back, > and retry). Then we should not mark the commit as started, since that could > potentially make the following GCO start too early, before we completed the > commit after the retry. > > This condition could trigger in some corner cases, where InnoDB would take > temporarily table/row locks that are released again immediately, not held > until the transaction commits. This happens with dict_stats updates and > possibly auto-increment locks. > > Such locks can be passed to thd_rpl_deadlock_check() and cause a deadlock > kill to be scheduled in the background. But since the blocking locks are > held only temporarily, they can be released before the background kill > happens. This way, the kill can be delayed until after mark_start_commit() > has been called. Thus we need to check the synchronous indication > rgi->killed_for_retry, not just the asynchroneous thd->killed. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > --- > sql/rpl_parallel.cc | 20 ++++++++++++++++---- > sql/rpl_rli.cc | 17 +++++++++++++++++ > 2 files changed, 33 insertions(+), 4 deletions(-) > > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index 1cfdf96ee3b..9c4222d7817 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -1450,11 +1450,23 @@ handle_rpl_parallel_thread(void *arg) > after mark_start_commit(), we have to unmark, which has at least a > theoretical possibility of leaving a window where it looks like all > transactions in a GCO have started committing, while in fact one > - will need to rollback and retry. This is not supposed to be possible > - (since there is a deadlock, at least one transaction should be > - blocked from reaching commit), but this seems a fragile ensurance, > - and there were historically a number of subtle bugs in this area. > + will need to rollback and retry. > + > + Normally this will not happen, since the kill is there to resolve a > + deadlock that is preventing at least one transaction from proceeding. > + One case it can happen is with InnoDB dict stats update, which can > + temporarily cause transactions to block each other, but locks are > + released immediately, they don't linger until commit. There could be > + other similar cases, there were historically a number of subtle bugs > + in this area. > + > + But once we start the commit, we can expect that no new lock > + conflicts will be introduced. So by handling any lingering deadlock > + kill at this point just before mark_start_commit(), we should be > + robust even towards spurious deadlock kills. > */ > + if (rgi->killed_for_retry != rpl_group_info::RETRY_KILL_NONE) > + wait_for_pending_deadlock_kill(thd, rgi); > if (!thd->killed) > { > DEBUG_SYNC(thd, "rpl_parallel_before_mark_start_commit"); > diff --git a/sql/rpl_rli.cc b/sql/rpl_rli.cc > index 9d4e09a362c..8284b07f7ff 100644 > --- a/sql/rpl_rli.cc > +++ b/sql/rpl_rli.cc > @@ -2518,6 +2518,23 @@ rpl_group_info::unmark_start_commit() > > e= this->parallel_entry; > mysql_mutex_lock(&e->LOCK_parallel_entry); > + /* > + Assert that we have not already wrongly completed this GCO and signalled > + the next one to start, only to now unmark and make the signal invalid. > + This is to catch problems like MDEV-34696. > + > + The error inject rpl_parallel_simulate_temp_err_xid is used to test this > + precise situation, that we handle it gracefully if it somehow occurs in a > + release build. So disable the assert in this case. > + */ > +#ifndef DBUG_OFF > + bool allow_unmark_after_complete= false; > + DBUG_EXECUTE_IF("rpl_parallel_simulate_temp_err_xid", > + allow_unmark_after_complete= true;); > + DBUG_ASSERT(!gco->next_gco || > + gco->next_gco->wait_count > e->count_committing_event_groups || > + allow_unmark_after_complete); > +#endif > --e->count_committing_event_groups; > mysql_mutex_unlock(&e->LOCK_parallel_entry); > }

1 0

Re: [PATCH] MDEV-34481 optimize away waiting for owned by prepared xa non-unique index record
by Kristian Nielsen 15 Aug '24

15 Aug '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> This is not the way to communicate back from engine to server. Instead, >> simply return a different error code than HA_ERR_LOCK_WAIT_TIMEOUT from >> InnoDB that you can check for here. You can maybe use one of the existing >> HA_ERR_* from include/my_base.h, or add a new one if necessary. > However, adding a new error is clearly more intrusive. The new error is completely handled inside the code you add for signalling the condition inside InnoDB locking code and handling it in replication row event apply. It will not be visible to users. So it is not intrusive to add a new one. Just be careful to not pick a number that conflicts with another entry in a later branch, if you do this in a GA release. Alternatively, you can use the existing HA_ERR_ROW_NOT_VISIBLE error, which is used in a somewhat similar way in storage/maria/ma_blockrec.c and nowhere else. But I would just add a new error for clarity. > And at the same time I thought a some sort of runtime state the slave > applier had always missed. So the intent of this struct is future-minded > as well. This is not the point. Even if the rgi->exec_flags already existed, it would still be wrong to use it for this. Here is the code where you are handling the condition: > + if (error == HA_ERR_LOCK_WAIT_TIMEOUT && !is_index_unique && > + (rgi->exec_flags & (1 << rpl_group_info::HIT_BUSY_INDEX))) > + { > + rgi->exec_flags &= ~(1 << rpl_group_info::HIT_BUSY_INDEX); > + skip_first_compare= true; So you _already_ are using a specific error code to signal the condition from InnoDB to replication, the error HA_ERR_LOCK_WAIT_TIMEOUT. But this error can also be thrown in other situations, so you need to add these extra conditions to distinguish. So essentially, you are using the wrong error code to signal the condition. Use a correct error code that uniquely identifies the condition that occured, and then you can avoid these extra conditions and simplify the code. > I would be fine with either method. Good, then please change it to return a proper error code. The only reason that you are using HA_ERR_LOCK_WAIT_TIMEOUT is that then you could do the quick change inside InnoDB to set its timeout value to 0, and this happens to then propagate the HA_ERR_LOCK_WAIT_TIMEOUT up to the callers without waiting. But that's not an appropriate way to design new code and APIs. Yes, it is frustrating when conceptually simple changes need extra effort, due to the need to work in an existing old and complex code base like MariaDB. But that is just the way it is, it is the same for all of us. - Kristian.

1 0