- developers - lists.mariadb.org

Re: fec3696fef6: MDEV-15751 CURRENT_TIMESTAMP should return a TIMESTAMP [WITH TIME ZONE?]
by Sergei Golubchik 29 Aug '24

29 Aug '24

Hi, Alexander, This has waited far too long :( > commit fec3696fef6 > Author: Alexander Barkov <bar(a)mariadb.com> > Date: Tue Feb 14 13:27:46 2023 +0400 > > diff --git a/sql/sql_class.h b/sql/sql_class.h > index f0bf695a1c1..7378225eaa2 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3900,7 +3900,8 @@ class THD: public THD_count, /* this must be first */ > } > const Type_handler *type_handler_for_datetime() const; > bool timestamp_to_TIME(MYSQL_TIME *ltime, my_time_t ts, > - ulong sec_part, date_mode_t fuzzydate); > + ulong sec_part, date_mode_t fuzzydate, > + bool zero_timesteamp_means_zero_datetime); I don't understand it. NOW() cannot return a zero timestamp, how this feature can possibly need changes to zero timestamp treatment? > inline my_time_t query_start() { return start_time; } > inline ulong query_start_sec_part() > { used|= QUERY_START_SEC_PART_USED; return start_time_sec_part; } > diff --git a/sql/item_timefunc.h b/sql/item_timefunc.h > index a5f6d9307c6..1c463c8a332 100644 > --- a/sql/item_timefunc.h > +++ b/sql/item_timefunc.h > @@ -707,6 +707,49 @@ class Item_datetimefunc :public Item_func > }; > > > +class Item_timestampfunc: public Item_func > +{ > +protected: > + Datetime to_datetime(THD *thd) > + { > + return Timestamp_or_zero_datetime_native_null(thd, this).to_datetime(thd); how does it differ from Datetime(this) ? > + } > +public: > + Item_timestampfunc(THD *thd): Item_func(thd) {} > + Item_timestampfunc(THD *thd, Item *a): Item_func(thd, a) {} > + const Type_handler *type_handler() const override > + { return &type_handler_timestamp2; } > + bool get_date(THD *thd, MYSQL_TIME *ltime, date_mode_t fuzzydate) override > + { > + return null_value= to_datetime(thd).copy_to_mysql_time(ltime); > + } > + double val_real() override > + { > + Datetime dt= to_datetime(current_thd); > + null_value= !dt.is_valid_datetime(); > + return dt.to_double(); may be `return null_value ? 0 : dt.to_double()` ? why to call to_double if it isn't a valid datetime? (same everywhere below) > + } > + longlong val_int() override > + { > + Datetime dt= to_datetime(current_thd); > + null_value= !dt.is_valid_datetime(); > + return dt.to_longlong(); > + } > + my_decimal *val_decimal(my_decimal *to) override > + { > + Datetime dt= to_datetime(current_thd); > + null_value= !dt.is_valid_datetime(); > + return dt.to_decimal(to); > + } > + String *val_str(String *to) override > + { > + Datetime dt= to_datetime(current_thd); > + null_value= !dt.is_valid_datetime(); > + return dt.to_string(to, decimals); > + } > +}; > + > + > /* Abstract CURTIME function. Children should define what time zone is used */ > > class Item_func_curtime :public Item_timefunc > @@ -837,20 +880,48 @@ class Item_func_now :public Item_datetimefunc > }; > > > -class Item_func_now_local :public Item_func_now > +/* > + This is a replacement for Item_func_now_local, > + returning TIMESTAMP instead of DATETIME. why did you rename it? why didn't you change Item_func_now_utc? > +*/ > +class Item_func_current_timestamp: public Item_timestampfunc > { > public: > - Item_func_now_local(THD *thd, uint dec): Item_func_now(thd, dec) {} > + Item_func_current_timestamp(THD *thd, uint dec) > + :Item_timestampfunc(thd) > + { decimals= dec; } why not Item_timestampfunc(thd, dec) ? > LEX_CSTRING func_name_cstring() const override > { > static LEX_CSTRING name= {STRING_WITH_LEN("current_timestamp") }; > return name; > } > - int save_in_field(Field *field, bool no_conversions) override; > - void store_now_in_TIME(THD *thd, MYSQL_TIME *now_time) override; > + void print(String *str, enum_query_type query_type) override > + { > + str->append(func_name_cstring()); > + str->append('('); > + if (decimals) > + str->append_ulonglong(decimals); > + str->append(')'); > + } > + bool fix_length_and_dec(THD *thd) override > + { > + if (check_decimal_scale_or_error(TIME_SECOND_PART_DIGITS)) > + return true; > + fix_attributes_datetime(decimals); > + return false; > + } > + bool val_native(THD *thd, Native *to) override; > + bool check_vcol_func_processor(void *arg) override > + { > + /* > + NOW is safe for replication as slaves will run with same time as > + master > + */ what does this replication comment do in check_vcol_func_processor? (I know it was in the old code too, but "old code did it" never stopped your cleanups before :) > + return mark_unsupported_function(func_name(), "()", arg, VCOL_TIME_FUNC); > + } > enum Functype functype() const override { return NOW_FUNC; } > Item *get_copy(THD *thd) override > - { return get_item_copy<Item_func_now_local>(thd, this); } > + { return get_item_copy<Item_func_current_timestamp>(thd, this); } > }; > > > diff --git a/sql/item_timefunc.cc b/sql/item_timefunc.cc > index 52b9ae7a682..3e224e3e6f7 100644 > --- a/sql/item_timefunc.cc > +++ b/sql/item_timefunc.cc > @@ -1642,32 +1633,10 @@ void Item_func_now::print(String *str, enum_query_type query_type) > } > > > -int Item_func_now_local::save_in_field(Field *field, bool no_conversions) > +bool Item_func_current_timestamp::val_native(THD *thd, Native *to) > { > - if (field->type() == MYSQL_TYPE_TIMESTAMP) > - { > - THD *thd= field->get_thd(); > - my_time_t ts= thd->query_start(); > - ulong sec_part= decimals ? thd->query_start_sec_part() : 0; > - sec_part-= my_time_fraction_remainder(sec_part, decimals); > - field->set_notnull(); > - field->store_timestamp(ts, sec_part); > - return 0; > - } > - else > - return Item_datetimefunc::save_in_field(field, no_conversions); > -} > - > - > -/** > - Converts current time in my_time_t to MYSQL_TIME representation for local > - time zone. Defines time zone (local) used for whole NOW function. > -*/ > -void Item_func_now_local::store_now_in_TIME(THD *thd, MYSQL_TIME *now_time) > -{ > - thd->variables.time_zone->gmt_sec_to_TIME(now_time, thd->query_start()); > - set_sec_part(thd->query_start_sec_part(), now_time, this); > - thd->used|= THD::TIME_ZONE_USED; > + Timestamp ts(Timeval(thd->query_start(), thd->query_start_sec_part())); > + return null_value= ts.trunc(decimals).to_native(to, decimals); can this possibly fail? > } > > > @@ -2732,7 +2693,6 @@ String *Item_func_tochar::val_str(String* str) > > bool Item_func_from_unixtime::fix_length_and_dec(THD *thd) > { > - thd->used|= THD::TIME_ZONE_USED; would be nice to have a test case for this. Like direct insert into a timestamp column no longer uses timezone - e.g. timestamp within dst switch time is inserted correctly. and vice versa, when from_unixtime is converted to a string, like `insert ... values(concat(from_unixtime(...)))` then TIME_ZONE_USED is still set and show binlog events (or mysqlbinlog) should show that the timezone was correctly binlogged, as before. > tz= thd->variables.time_zone; > Type_std_attributes::set( > Type_temporal_attributes_not_fixed_dec(MAX_DATETIME_WIDTH, Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

MariaDB patches in Debian
by Otto Kekäläinen 27 Aug '24

27 Aug '24

Hi! Could perhaps Tuukka or Daniel review the patches currently carried in Debian if some of them would be applicable to be imported or re-implemented directly upstream so they can be dropped in Debian? https://udd.debian.org/patches.cgi?src=mariadb&version=1%3A11.4.3-1 Thanks

1 0

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

Can we get MariaDB GitHub CI to consistently be green?
by Otto Kekäläinen 10 Aug '24

10 Aug '24

Hi! Thanks to everyone who has worked on polishing the CI tests and integrations. Looking at e.g. 10.11 branch[1] I see that commit ccb7a1e[2] has a green checkmark next to it and all 15 CI jobs passed. I just wanted to check if everyone developing the MariaDB Server is committed to getting the CI to be consistently green? This means that GitHub rules need to be a bit more strict, not allowing any failing tests jobs at all, and developers and managers need to agree to stop adding new commits on any release branch if it can't be done without a fully passing CI run. Thanks to Daniel's review on latest failures we know these are at the moment recurring: * MDEV-25614 Galera test failure on GCF-354 included in MDEV-33073 always green buildbot * MDEV-33785 aarch64 macos encryption.create_or_replace * MDEV-33601 galera_3nodes.galera_safe_to_bootstrap test failing * MDEV-33786 galera_3nodes.galera_vote_rejoin_mysqldump mysql_shutdown failed at line 85: I see two approaches to get to consistently green CI: 1) Stop all development and focus on just fixing these, don't continue until CI is fully green, and once it is fully green make the GitHub branch protection settings one notch stricter to not allow any new commits unless the CI is fully green so it never regresses again. 2) Disable these tests and make the rules in GitHub branch protection one notch stricter right away, and not allow any new commits unless the CI is fully green ensuring no new recurring failures are introduced. - Otto [1] https://github.com/MariaDB/server/commits/10.11 [2] https://github.com/MariaDB/server/commit/ccb7a1e9a15e6a47aba97f9bdbfab2e4bf…

7 12

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

01 Aug '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Indeed a combination of the engine locking protocol and the slave retry > applies correctly to IDEMPOTENT mode. >> The optimistic parallel replication should work correctly with >> IDEMPOTENT, > Indeed. Ok good to know. It's definitely a tricky issue, but apparently no known bugs, at least to the two of us. As you saw, I did have to try a test case to feel confident it works ;-) - Kristian.

1 0

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

31 Jul '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: > Kristian Nielsen <knielsen(a)knielsen-hq.org> writes: >> You need to make sure that --slave_exec_mode=IDEMPOTENT is not used. >> Otherwise you can get silent data corruption. > > IDEMPOTENT is always risky. Here it would add up another bit, in the > parallel mode. What do you mean "IDEMPOTENT is always risky"? As far as I know, if the slave would replicate without error in STRICT, it will replicate exactly the same way in IDEMPOTENT. Only if there is an error in STRICT mode will the IDEMPOTENT mode do something different (ignore the error in some cases). So the risk is that we might fail to detect that the slave is diverged from the master, right? Note that diverged slave can be undetected even in STRICT mode in many cases. > Btw should IDEMPOTENT be disallowed in the parallel mode actually? No, why? IDEMPOTENT should work just as documented in all parallel replication modes. Or do you know of any bug with IDEMPOTENT and optimistic parallel replication that I don't know of? If so do you have a test case? The optimistic parallel replication should work correctly with IDEMPOTENT, since the second query that's ignoring the error should still be taking InnoDB row locks, which will conflict as normal with the first transaction and cause the second to be retried, this time without (ignored) error. I just tried with the below testcase, it seems to work. Only when you fiddle with the InnoDB locking like in your patch and ignore certain locks, do you get problems, and then you can end up with a later transaction completely ignoring a lock that is temporarily held by an earlier XA PREPARE (even though the corresponding XA COMMIT is also earlier). And this will result in incorrect replication and diverged slave, I think (I did not test). This is not "risky", this is incorrect behaviour and a serious bug. So it seems you will need to maybe give an error when replicating any XA transaction if slave_exec_mode=IDEMPOTENT? Or handle it some other way to avoid silently replicating incorrectly. As you know, my opinion is that this is just yet another symptom of the real issue of MDEV-32020, but I know you're going to ignore that. So I've given up caring about what happens to XA replication, as long as it doesn't negatively affect non-XA stuff. > I just don't see any its usefullness except creating a chaotic state. Why do you think it creates a "chaotic state"? - Kristian. ----------------------------------------------------------------------- --source include/have_innodb.inc --source include/have_binlog_format_row.inc --source include/master-slave.inc --connection master CALL mtr.add_suppression("Can't find record in 't1'"); ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; CREATE TABLE t1 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; CREATE TABLE t2 (a INT PRIMARY KEY) ENGINE=InnoDB; BEGIN; INSERT INTO t1 VALUES (1, 1); INSERT INTO t1 VALUES (2, 1); INSERT INTO t1 VALUES (3, 1); INSERT INTO t1 VALUES (4, 1); INSERT INTO t1 VALUES (5, 1); COMMIT; --sync_slave_with_master --source include/stop_slave.inc SET @old_exec_mode= @@GLOBAL.slave_exec_mode; SET GLOBAL slave_exec_mode=IDEMPOTENT; SET @old_parallel_mode= @@GLOBAL.slave_parallel_mode; SET GLOBAL slave_parallel_mode= optimistic; SET @old_threads= @@GLOBAL.slave_parallel_threads; SET GLOBAL slave_parallel_threads= 8; --source include/start_slave.inc # Block certain queries on the slave temporarily. BEGIN; INSERT INTO t2 VALUES (1), (2), (3); --connection master UPDATE t1 SET b=10 WHERE a=1; BEGIN; INSERT INTO t2 VALUES (1); # A DELETE to make room for a later INSERT, will be delayed a bit on slave. DELETE FROM t1 WHERE a=2; COMMIT; UPDATE t1 SET b=20 WHERE a=3; BEGIN; INSERT INTO t2 VALUES (2); # An INSERT to be deleted again shortly, will be delayed a bit on slave. INSERT INTO t1 VALUES (6,2); COMMIT; UPDATE t1 SET b=30 WHERE a=4; # This INSERT cannot run until after the earlier DELETE. INSERT INTO t1 VALUES (2, 1); # This DELETE cannot run until after the earlier INSERT. DELETE FROM t1 WHERE a=6; SELECT * FROM t1 ORDER BY a; --save_master_pos --connection slave --sleep 1 ROLLBACK; --sync_with_master SELECT * FROM t1 ORDER BY a; --source include/stop_slave.inc SET GLOBAL slave_exec_mode= @old_exec_mode; SET GLOBAL slave_parallel_mode= @old_parallel_mode; SET GLOBAL slave_parallel_threads= @old_threads; --source include/start_slave.inc --connection master DROP TABLE t1, t2; --source include/rpl_end.inc

1 0

Re: 1287c901: TLS/SSL changes (major rework)
by Sergei Golubchik 31 Jul '24

31 Jul '24

Hi, Georg, On Jul 31, Georg Richter wrote: > revision-id: 1287c901 (v3.4.0-5-g1287c901) > parent(s): 5386f1a3 > author: Georg Richter > committer: Georg Richter > timestamp: 2024-07-16 13:12:26 +0200 > message: > > TLS/SSL changes (major rework) > diff --git a/include/ma_common.h b/include/ma_common.h > index dfa96621..dc900b0d 100644 > --- a/include/ma_common.h > +++ b/include/ma_common.h > @@ -131,15 +131,9 @@ typedef struct st_mariadb_field_extension > MARIADB_CONST_STRING metadata[MARIADB_FIELD_ATTR_LAST+1]; /* 10.5 */ > } MA_FIELD_EXTENSION; > > -#if defined(HAVE_SCHANNEL) || defined(HAVE_GNUTLS) > -#define reset_tls_self_signed_error(mysql) \ > - do { \ > - free((char*)mysql->net.tls_self_signed_error); \ > - mysql->net.tls_self_signed_error= 0; \ > - } while(0) > -#else > -#define reset_tls_self_signed_error(mysql) \ > - do { \ > - mysql->net.tls_self_signed_error= 0; \ > +#ifdef HAVE_TLS > +#define reset_tls_error(mysql) \ > + do { \ > + mysql->net.tls_verify_status= 0; \ you've lost the original error message when you've changed char* pointer to the message to my_bool flag. > } while(0) > #endif > diff --git a/include/ma_tls.h b/include/ma_tls.h > index 23f53560..444ea4aa 100644 > --- a/include/ma_tls.h > +++ b/include/ma_tls.h > @@ -134,6 +141,7 @@ const char *ma_tls_get_cipher(MARIADB_TLS *ssl); > hash_type hash_type as defined in ma_hash.h > fp buffer for fingerprint > fp_len buffer length > + my_bool verify_period I don't see any 'my bool verify_period' in the list of arguments > > Returns: > actual size of finger print > @@ -150,7 +158,7 @@ unsigned int ma_tls_get_finger_print(MARIADB_TLS *ctls, uint hash_type, char *fp > int ma_tls_get_protocol_version(MARIADB_TLS *ctls); > const char *ma_pvio_tls_get_protocol_version(MARIADB_TLS *ctls); > int ma_pvio_tls_get_protocol_version_id(MARIADB_TLS *ctls); > -unsigned int ma_tls_get_peer_cert_info(MARIADB_TLS *ctls); > +unsigned int ma_tls_get_peer_cert_info(MARIADB_TLS *ctls, unsigned int size); is this part of the public API? > void ma_tls_set_connection(MYSQL *mysql); > > /* Function prototypes */ > diff --git a/include/mysql.h b/include/mysql.h > index 6c84a462..ba92527d 100644 > --- a/include/mysql.h > +++ b/include/mysql.h > @@ -448,6 +449,16 @@ typedef struct st_mysql_time > #define MYSQL_WAIT_EXCEPT 4 > #define MYSQL_WAIT_TIMEOUT 8 > > +#define MARIADB_TLS_VERIFY_OK 0 > +#define MARIADB_TLS_VERIFY_TRUST 1 > +#define MARIADB_TLS_VERIFY_HOST 2 we've agreed to swap two previous lines > +#define MARIADB_TLS_VERIFY_PERIOD 4 > +#define MARIADB_TLS_VERIFY_FINGERPRINT 8 is that right? you want to allow fingerprints even if the cert is expired? > +#define MARIADB_TLS_VERIFY_REVOKED 16 > +#define MARIADB_TLS_VERIFY_UNKNOWN 32 > +#define MARIADB_TLS_VERIFY_ERROR 128 /* last */ > + > + > typedef struct character_set > { > unsigned int number; /* character set number */ > diff --git a/libmariadb/ma_tls.c b/libmariadb/ma_tls.c > index 1bda7dcf..f6ea6661 100644 > --- a/libmariadb/ma_tls.c > +++ b/libmariadb/ma_tls.c > @@ -103,9 +103,54 @@ my_bool ma_pvio_tls_close(MARIADB_TLS *ctls) > return ma_tls_close(ctls); > } > > -int ma_pvio_tls_verify_server_cert(MARIADB_TLS *ctls) > +int ma_pvio_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int flags) > { > - return ma_tls_verify_server_cert(ctls); > + MYSQL *mysql; > + int rc; > + > + if (!ctls || !ctls->pvio || !ctls->pvio->mysql) > + return 0; > + > + mysql= ctls->pvio->mysql; > + > + /* Skip peer certificate verification */ > + if (ctls->pvio->mysql->options.extension->tls_allow_invalid_server_cert) > + { > + return 0; > + } > + > + rc= ma_tls_verify_server_cert(ctls, flags); > + > + /* Set error messages */ > + if (!mysql->net.last_errno) > + { > + if (mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_PERIOD) > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Certificate not yet valid or expired"); > + else if (mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_FINGERPRINT) > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Fingerprint validation of peer certificate failed"); > + else if (mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_REVOKED) > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Certificate revoked"); > + else if (mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_HOST) > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Hostname verification failed"); > + else if (mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_UNKNOWN) > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Peer certificate verification failed"); > + else if (mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_TRUST) > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Peer certificate is not trusted"); let's try to preserve the error as set by the ssl library here > + } > + > + return rc; > } > > const char *ma_pvio_tls_cipher(MARIADB_TLS *ctls) > diff --git a/plugins/auth/my_auth.c b/plugins/auth/my_auth.c > index faea968c..e7429fae 100644 > --- a/plugins/auth/my_auth.c > +++ b/plugins/auth/my_auth.c > @@ -382,14 +419,30 @@ static int send_client_reply_packet(MCPVIO_EXT *mpvio, > errno); > goto error; > } > + mysql->net.tls_verify_status = 0; > if (ma_pvio_start_ssl(mysql->net.pvio)) > goto error; > - if (mysql->net.tls_self_signed_error && > - (!mysql->passwd || !mysql->passwd[0] || !hashing(mpvio->plugin))) > - { > - /* cannot use auth to validate the cert */ > - set_error_from_tls_self_signed_error(mysql); > - goto error; > + > + verify_flags= MARIADB_TLS_VERIFY_PERIOD | MARIADB_TLS_VERIFY_REVOKED; > + if (have_fingerprint(mysql)) > + { > + verify_flags|= MARIADB_TLS_VERIFY_FINGERPRINT; > + } else { > + verify_flags|= MARIADB_TLS_VERIFY_TRUST | MARIADB_TLS_VERIFY_HOST; > + } > + > + if (ma_pvio_tls_verify_server_cert(mysql->net.pvio->ctls, verify_flags) > MARIADB_TLS_VERIFY_OK) > + { > + if (mysql->net.tls_verify_status > MARIADB_TLS_VERIFY_TRUST || > + (mysql->options.ssl_ca || mysql->options.ssl_capath)) > + goto error; > + > + if (is_local_connection(mysql->net.pvio)) I'd say this should be earlier. Like if (!is_local_connection(mysql->net.pvio)) if (have_fingerprint(mysql)) { verify_flags|= MARIADB_TLS_VERIFY_FINGERPRINT; } else { verify_flags|= MARIADB_TLS_VERIFY_TRUST | MARIADB_TLS_VERIFY_HOST; } > + { > + CLEAR_CLIENT_ERROR(mysql); > + } > + else if (!password_and_hashing(mysql, mpvio->plugin)) > + goto error; > } > } > #endif /* HAVE_TLS */ > @@ -769,8 +822,14 @@ retry: > auth_plugin= &dummy_fallback_client_plugin; > > /* can we use this plugin with this tls server cert ? */ > - if (mysql->net.tls_self_signed_error && !hashing(auth_plugin)) > - return set_error_from_tls_self_signed_error(mysql); > + if ((mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_TRUST) && > + !password_and_hashing(mysql, auth_plugin)) > + { > + my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > + ER(CR_SSL_CONNECTION_ERROR), > + "Certificate verification failure: The certificate is NOT trusted."); already commented about using library error message > + return 1; > + } > goto retry; > } > /* > @@ -782,7 +841,9 @@ retry: > if (ma_read_ok_packet(mysql, mysql->net.read_pos + 1, pkt_length)) > return -1; > > - if (!mysql->net.tls_self_signed_error) > + if (!mysql->net.tls_verify_status || > + ((mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_TRUST) && > + is_local_connection(mysql->net.pvio))) Not needed, see above > return 0; > > assert(mysql->options.use_ssl); > diff --git a/libmariadb/secure/openssl.c b/libmariadb/secure/openssl.c > index 8231a244..b5b573a9 100644 > --- a/libmariadb/secure/openssl.c > +++ b/libmariadb/secure/openssl.c > @@ -459,24 +461,38 @@ error: > return NULL; > } > > -unsigned int ma_tls_get_peer_cert_info(MARIADB_TLS *ctls) > +unsigned int ma_tls_get_peer_cert_info(MARIADB_TLS *ctls, uint hash_size) > { > X509 *cert; > + unsigned int hash_alg; > SSL *ssl; > + char fp[129]; > > + switch (hash_size) { > + case 0: > + case 256: > + hash_alg= MA_HASH_SHA256; > + break; > + case 384: > + hash_alg= MA_HASH_SHA384; > + break; > + case 512: > + hash_alg= MA_HASH_SHA512; > + break; > + default: > + return 1; > + } > + > if (!ctls || !ctls->ssl) > return 1; > > - /* Did we already read peer cert information ? */ > - if (ctls->cert_info.version) > - return 0; > - > ssl= (SSL *)ctls->ssl; > > /* Store peer certificate information */ > + if (!ctls->cert_info.version) > + { when can it be already set? > if ((cert= SSL_get_peer_certificate(ssl))) > { > - char fp[33]; > #if OPENSSL_VERSION_NUMBER >= 0x10101000L > const ASN1_TIME *not_before= X509_get0_notBefore(cert), > *not_after= X509_get0_notAfter(cert); > @@ -714,9 +714,40 @@ my_bool ma_tls_close(MARIADB_TLS *ctls) > return rc; > } > > -int ma_tls_verify_server_cert(MARIADB_TLS *ctls) > +/** Check for possible errors, and store the result in net.tls_verify_status. > + verification will happen after handshake by ma_tls_verify_server_cert(). > + To retrieve all errors, this callback function returns always true. > + (By default OpenSSL stops verification after first error > +*/ > +static int ma_verification_callback(int preverify_ok __attribute__((unused)), X509_STORE_CTX *ctx) > { > - X509 *cert; > + SSL *ssl; > + > + int x509_err= X509_STORE_CTX_get_error(ctx); > + > + if ((ssl = X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx()))) > + { > + MYSQL *mysql= (MYSQL *)SSL_get_app_data(ssl); > + > + if ((x509_err == X509_V_ERR_DEPTH_ZERO_SELF_SIGNED_CERT || > + x509_err == X509_V_ERR_SELF_SIGNED_CERT_IN_CHAIN)) > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_TRUST; > + else if (x509_err == X509_V_ERR_CERT_REVOKED) > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_REVOKED; > + else if (x509_err == X509_V_ERR_CERT_NOT_YET_VALID || > + x509_err == X509_V_ERR_CERT_HAS_EXPIRED) > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_PERIOD; > + else if (x509_err != X509_V_OK) > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_UNKNOWN; and here you need to store the error message in mysql->net. for example, else if (x509_err == X509_V_ERR_CERT_NOT_YET_VALID || x509_err == X509_V_ERR_CERT_HAS_EXPIRED) { if (mysql->net.tls_verify_status < MARIADB_TLS_VERIFY_PERIOD) mysql->net.tls_verify_error= X509_verify_cert_error_string(x509_err); mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_PERIOD; } or else if (x509_err == X509_V_ERR_CERT_NOT_YET_VALID || x509_err == X509_V_ERR_CERT_HAS_EXPIRED) { if (mysql->net.tls_verify_status < MARIADB_TLS_VERIFY_PERIOD) my_set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, ER(CR_SSL_CONNECTION_ERROR), X509_verify_cert_error_string(x509_err)); mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_PERIOD; } > + } > + > + /* continue verification */ > + return 1; > +} > + > +int ma_tls_verify_server_cert(MARIADB_TLS *ctls, unsigned int verify_flags) > +{ > + X509 *cert= NULL; > MYSQL *mysql; > SSL *ssl; > MARIADB_PVIO *pvio; > @@ -734,52 +765,97 @@ int ma_tls_verify_server_cert(MARIADB_TLS *ctls) > mysql= (MYSQL *)SSL_get_app_data(ssl); > pvio= mysql->net.pvio; > > + if (verify_flags & MARIADB_TLS_VERIFY_FINGERPRINT) > + { > + if (ma_pvio_tls_check_fp(ctls, mysql->options.extension->tls_fp, mysql->options.extension->tls_fp_list)) > + { > + mysql->net.tls_verify_status |= MARIADB_TLS_VERIFY_FINGERPRINT; > + return 1; > + } > + > + /* if certificates are valid and no revocation error occured, > + we can return */ > + if (!(mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_PERIOD) && > + !(mysql->net.tls_verify_status & MARIADB_TLS_VERIFY_REVOKED)) you can check this before trying the fingerprint > + { > + mysql->net.tls_verify_status= MARIADB_TLS_VERIFY_OK; > + return 0; > + } > + } > + > + if (mysql->net.tls_verify_status & verify_flags) > + { > + return 1; > + } > + > + if (verify_flags & MARIADB_TLS_VERIFY_HOST) > + { > if (!mysql->host) > { > pvio->set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > ER(CR_SSL_CONNECTION_ERROR), "Invalid (empty) hostname"); > - return 1; > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > + return MARIADB_TLS_VERIFY_ERROR; > } > > if (!(cert= SSL_get_peer_certificate(ssl))) > { > pvio->set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > ER(CR_SSL_CONNECTION_ERROR), "Unable to get server certificate"); > - return 1; > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; not sure it's MARIADB_TLS_VERIFY_HOST, may be MARIADB_TLS_VERIFY_UNKNOWN ? > + return MARIADB_TLS_VERIFY_ERROR; > } > + > #ifdef HAVE_OPENSSL_CHECK_HOST > if (X509_check_host(cert, mysql->host, strlen(mysql->host), 0, 0) != 1 > && X509_check_ip_asc(cert, mysql->host, 0) != 1) > + { > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > goto error; > + } > #else > x509sn= X509_get_subject_name(cert); > > if ((cn_pos= X509_NAME_get_index_by_NID(x509sn, NID_commonName, -1)) < 0) > + { > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > goto error; > + } > > if (!(cn_entry= X509_NAME_get_entry(x509sn, cn_pos))) > + { > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > goto error; > + } > > if (!(cn_asn1 = X509_NAME_ENTRY_get_data(cn_entry))) > + { > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > goto error; > + } > > cn_str = (char *)ASN1_STRING_data(cn_asn1); > > /* Make sure there is no embedded \0 in the CN */ > if ((size_t)ASN1_STRING_length(cn_asn1) != strlen(cn_str)) > + { > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > goto error; > + } > > if (strcmp(cn_str, mysql->host)) > + { > + mysql->net.tls_verify_status|= MARIADB_TLS_VERIFY_HOST; > goto error; > + } > #endif > X509_free(cert); > - > + } > return 0; > error: > + if (cert) > X509_free(cert); > > - pvio->set_error(mysql, CR_SSL_CONNECTION_ERROR, SQLSTATE_UNKNOWN, > - ER(CR_SSL_CONNECTION_ERROR), "Validation of SSL server certificate failed"); > return 1; > } > Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

1 0

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

31 Jul '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> Also, are you aware that when you skip a record like this, you may be >> skipping the very record that needs to be changed? > Yet luckily this is covered by the BASE (of the patch) logics. > After(needless) scan the rest of duplicate key record set the third group > of events would have to retry, to success. (This is optimizable, but I You need to make sure that --slave_exec_mode=IDEMPOTENT is not used. Otherwise you can get silent data corruption. - Kristian.

1 0

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

31 Jul '24

> From: Andrei <andrei.elkin(a)mariadb.com> > > This work partly implements a ROW binlog format MDEV-33455 part > that is makes non-unique-index-only table XA replication safe in RBR. > Existence of at least one non-null unique index has always guaranteed > safety (no hang to error). Hi Andrei, A couple early comments on your MDEV-34481 patch, about things that affect non-XA parts of the code and will need to be changed: > diff --git a/sql/sql_class.cc b/sql/sql_class.cc > index 6419a58fbe4..b2321cfd865 100644 > --- a/sql/sql_class.cc > +++ b/sql/sql_class.cc > @@ -5444,20 +5449,33 @@ thd_need_wait_reports(const MYSQL_THD thd) > transaction. > */ > extern "C" int > -thd_rpl_deadlock_check(MYSQL_THD thd, MYSQL_THD other_thd) > +thd_rpl_deadlock_check(MYSQL_THD thd, MYSQL_THD other_thd, > + bool is_other_prepared, bool is_index_unique, > + uint *flagged) > + > + /* > + Return to the caller the fact of the non-unique index wait lock > + conflicts with one of a prepared state transaction. > + */ > + if (!is_index_unique && rgi && rgi->is_row_event_execution()) { > + if (is_other_prepared && !other_thd) > + { > + rgi->exec_flags |= 1 << rpl_group_info::HIT_BUSY_INDEX; > + (*flagged)++; > + } > + } > + No, thd_rpl_deadlock_check() has a specific purpose related to handling deadlock between active transactions. Don't add this XA-specific and unrelated logic to this function. Instead, add a separate function that InnoDB can call in the specific case you need. That function then only needs the parameters `thd` and `flagged`, the others are not needed: extern "C" bool thd_xa_skip_lock_check(MYSQL_THD thd, uint *flagged) { return (thd->rgi_slave && thd->rgi_slave->us_row_event_execution()); } Then in InnoDB, call something like this: if (lock->trx->state == TRX_STATE_PREPARED && (!lock->index->is_unique() || lock->index->n_nullable)) count_non_unique_index_hit++; About the HIT_BUSY_INDEX, see here: > diff --git a/sql/log_event_server.cc b/sql/log_event_server.cc > index f3d01b7d9ed..166295ed263 100644 > --- a/sql/log_event_server.cc > +++ b/sql/log_event_server.cc > @@ -8149,11 +8154,20 @@ int Rows_log_event::find_row(rpl_group_info *rgi) > HA_READ_KEY_EXACT)))) > { > DBUG_PRINT("info",("no record matching the key found in the table")); > - if (error == HA_ERR_KEY_NOT_FOUND) > - error= row_not_found_error(rgi); > - table->file->print_error(error, MYF(0)); > - table->file->ha_index_end(); > - goto end; > + 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; > + } Here, you are effectively making a decision in the storage engine to skip this record which is locked by an XA PREPAREd transaction. But you are communicating this back to the server in a very convoluted way, by introducing the rgi->exec_flags and setting HIT_BUSY_INDEX. 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. if (error == HA_ERR_<something>) skip_first_compare= true; > diff --git a/sql/rpl_rli.h b/sql/rpl_rli.h > index 3f24481839b..a9ec44b58cb 100644 > --- a/sql/rpl_rli.h > +++ b/sql/rpl_rli.h > @@ -833,6 +833,15 @@ struct rpl_group_info > RETRY_KILL_KILLED > }; > uchar killed_for_retry; > + enum enum_exec_flags { > + HIT_BUSY_INDEX= 0 > + }; > + /* > + Being executed replication event's context. It could contain > + notification from engine such as attempts to lock already acquired > + index record. > + */ > + uint32 exec_flags; ... and then you don't need to add this. Also, are you aware that when you skip a record like this, you may be skipping the very record that needs to be changed? For example a case like this, with three transactions in order: XA START 'T1'; UPDATE my_table SET value=10 WHERE key=2 XA END 'T1'; XA PREPARE 'T1'; XA COMMIT 'T1'; XA START 'T2'; UPDATE my_table SET value=20 WHERE key=2; Transaction 'T2' may run speculatively in optimistic parallel replication after prepare of T1, but before commit of T1. Then it will skip the record key=2, IIUC. I didn't see any explanation of this case in the patch commit message or comments. Also I don't see this case tested in the test case, that needs to be added I believe. - Kristian.

1 0

Re: [MariaDB commits] [PATCH] MDEV-33856 New definition for Seconds_Behind_Master
by Kristian Nielsen 25 Jul '24

25 Jul '24

Hi Monty, Here is my review of the MDEV-33856 patch: > commit cc516f4d9913d732065c7abef983df42e5a8bab3 (origin/bb-11.6-MDEV-33856-seconds_behind_master) > Author: Monty <monty(a)mariadb.org> > Date: Tue May 14 23:47:59 2024 +0300 > > MDEV-33856 New definition for Seconds_Behind_Master If I understand the patch correctly, in introduces a new `Master_slave_time_diff` value as an alternative to the traditional Seconds_behind_master, with a clearer semantics. And does not change how Seconds_behind_master works. I think this is fine, and can help to preserve backwards compatibility. Just maybe mention this in the commit message, and/or change the bug title to "Alternative definition for Seconds_Behind_Master". > diff --git a/mysql-test/suite/perfschema/t/relaylog.test b/mysql-test/suite/perfschema/t/relaylog.test > index 12fc96a8b27..16f72efae98 100644 > --- a/mysql-test/suite/perfschema/t/relaylog.test > +++ b/mysql-test/suite/perfschema/t/relaylog.test > @@ -43,7 +43,7 @@ select > if (sum_number_of_bytes_read > 0, "MANY", "NONE") as SUM_NUMBER_OF_BYTES_READ, > if (sum_number_of_bytes_write > 0, "MANY", "NONE") as SUM_NUMBER_OF_BYTES_WRITE > from performance_schema.file_summary_by_instance > - where file_name like "%master-%" order by file_name; > + where file_name like "%master-%" and file_name not like "%sql/share%" order by file_name; > > select * from performance_schema.file_summary_by_instance > where file_name like "%slave-%" order by file_name; > @@ -96,7 +96,7 @@ sync_slave_with_master; > -- echo "============ Performance schema on slave ============" > > select * from performance_schema.file_summary_by_instance > - where file_name like "%master-%" order by file_name; > + where file_name like "%master-%" and file_name not like "%sql/share%" order by file_name; > > select > substring(file_name, locate("slave-", file_name)) as FILE_NAME, What is the reason for these changes (adding match on "%sql/share%" ? I don't understand the relation to the subject of the patch. > diff --git a/mysql-test/suite/rpl/t/master_last_event_time.inc b/mysql-test/suite/rpl/t/master_last_event_time.inc > new file mode 100644 > index 00000000000..0790d557a7b > --- /dev/null > +++ b/mysql-test/suite/rpl/t/master_last_event_time.inc > > +--connection server_3 > +--sleep 1 > +--source include/start_slave.inc > +--source include/sync_with_master_gtid.inc What is the purpose of --sleep 1 here? Usually it's wrong to have sleeps in the tests. Sometimes it is correct, but then there should be a comment explaining why a sleep is necessary (and sufficient) here. This sleep looks wrong, since there's a proper wait immediately after for the slave to sync up. > diff --git a/mysql-test/suite/rpl/t/master_last_event_time_stmt.test b/mysql-test/suite/rpl/t/master_last_event_time_stmt.test > new file mode 100644 > index 00000000000..fe0f16a3db3 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/master_last_event_time_stmt.test > @@ -0,0 +1,84 @@ > +# > +# Statement specific tests for master_last_event_time > +# > +--source include/have_binlog_format_statement.inc > > +# Slave will replicate the above with a row event which will be very fast > +# compared to the master event. I don't understand this comment. Why will the slave replicate using a row event, when the test case sets binlog_format=statement? > diff --git a/sql/log.cc b/sql/log.cc > index 1cc0fa77f08..63d9c756911 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -1898,10 +1898,20 @@ static inline int > binlog_commit_flush_xid_caches(THD *thd, binlog_cache_mngr *cache_mngr, > bool all, my_xid xid) > Xid_log_event end_evt(thd, xid, TRUE); > - return (binlog_flush_cache(thd, cache_mngr, &end_evt, all, TRUE, TRUE)); > + if (!thd->slave_thread && ! thd->user_time.val) > + { > + /* > + Ensure that on the master the event time is the time of commit, > + not the start of statement time. > + */ > + my_hrtime_t hrtime= my_hrtime(); > + end_evt.when= hrtime_to_my_time(hrtime); > + } > + DBUG_RETURN(binlog_flush_cache(thd, cache_mngr, &end_evt, all, TRUE, TRUE)); Ok, so correct me if I'm wrong, but I think the purpose here is: You want to have the time when the transaction ended (in case of long-running query). But for auto-committed query, there is no explicit COMMIT query, so the thd->start_time is the start of the transaction. So here you ensure that the XID_EVENT will have the time at start of commit in either case. If this is correct, probably could use a comment explaining this. Some comments: 1. It is better to check for thd->rgi_slave being non-NULL than for thd->slave_thread. The thd->rgi_slave is associated with the replication of some event or transaction. The thd->slave_thread just identifies the thread/connection. (This is mostly style/code clarity, as in current code the two values would track each other.) 2. Why the condition on thd->user_time.val? What is the situation where we are not replicating an event, but user_time is set and we do not want to put the current time in the XID_EVENT? 3. This handles an XID end event, which is for InnoDB transactions. But MyISAM/Aria use a different end event, a COMMIT query, and there are are few other cases as well. Don't you want consistently to have the end of the query as the time in all end events? Then this code should probably instead be in binlog_flush_cache() or perhaps write_transaction_to_binlog()? 4. Even given (3), there are still cases (eg. DDL) where there is no end event, and thus there is no event containing the time for the end of the operation on the master. Won't this cause some inconsistency in how the slave determines and displays its timings with this patch? If not, why not? > for (; has_xid && !entry.need_unlog && ha_info; ha_info= ha_info->next()) > { > if (ha_info->is_started() && ha_info->ht() != binlog_hton && > !ha_info->ht()->commit_checkpoint_request) > + { > entry.need_unlog= true; > + break; > + } > } Agree, this code always irritated me ;-) You could also move the condition (!entry.need_unlog && ha_info) out of the loop in an outer if () to not have to check it on every loop iteration (just an observation, entirely up to you, it's not something your patch introduced). > diff --git a/sql/log_event.cc b/sql/log_event.cc > index 3802f76efe0..706a4903eb0 100644 > --- a/sql/log_event.cc > +++ b/sql/log_event.cc > @@ -1104,10 +1116,12 @@ Log_event* Log_event::read_log_event(const uchar *buf, uint event_len, > switch(event_type) { > case QUERY_EVENT: > ev= new Query_log_event(buf, event_len, fdle, QUERY_EVENT); > + set_orig_exec_time_in_thd(((Query_log_event*) ev)->exec_time); > break; > case QUERY_COMPRESSED_EVENT: > ev= new Query_compressed_log_event(buf, event_len, fdle, > - QUERY_COMPRESSED_EVENT); > + QUERY_COMPRESSED_EVENT); > + set_orig_exec_time_in_thd(((Query_compressed_log_event*) ev)->exec_time); > @@ -964,6 +964,18 @@ Log_event* Log_event::read_log_event(IO_CACHE* file, > DBUG_RETURN(res); > } > > +/* > + Update thd->orig_exec_time > +*/ > + > +inline void set_orig_exec_time_in_thd(my_time_t exec_time) > +{ > +#if !defined(MYSQL_CLIENT) && defined(HAVE_REPLICATION) > + THD *thd= current_thd; > + if (likely(thd)) > + thd->orig_exec_time= exec_time; > +#endif > +} Using current_thd doesn't seem right here, and in fact it's not, as seen by this part of the patch where thd->orig_exec_time has to be set again for parallel replication: > diff --git a/sql/rpl_parallel.cc b/sql/rpl_parallel.cc > index 51ca3b183ec..d279761767a 100644 > --- a/sql/rpl_parallel.cc > +++ b/sql/rpl_parallel.cc > @@ -60,7 +60,10 @@ rpt_handle_event(rpl_parallel_thread::queued_event *qev, > rgi->future_event_relay_log_pos= qev->future_event_relay_log_pos; > strcpy(rgi->future_event_master_log_name, qev->future_event_master_log_name); > if (event_can_update_last_master_timestamp(ev)) > + { > rgi->last_master_timestamp= ev->when + ev->exec_time; > + thd->orig_exec_time= ev->exec_time; > + } So let's not set thd->orig_exec_time from Log_event::read_log_event(), it's redundant (for parallel replication), and logically setting it should be done when applying the event, not when reading the event. Instead, set the thd->orig_exec_time in exec_relay_log_event(), so we retain similarity with how replication is done in the parallel and non-parallel case. There is already a similar if (event_can_update_last_master_timestamp(ev)) in exec_relay_log_event(). (Also see comments below about moving this from THD to rpl_group_info to not increase THD just for this very specific case). > + > +/* > + Get the time when the event had been executed on the master. > + This works for both query events and load data events. > +*/ > + > +#if Q_EXEC_TIME_OFFSET != L_EXEC_TIME_OFFSET > +#error "Q_EXEC_TIME_OFFSET is not same as L_EXEC_TIME_OFFSET" > +#endif > + > +time_t query_event_get_time(const uchar *buf, > + const Format_description_log_event > + *description_event) > +{ > + time_t when= uint4korr(buf); > + buf+= description_event->common_header_len; > + return when + uint4korr(buf + Q_EXEC_TIME_OFFSET); Suggestion: Add an assert here: DBUG_ASSERT(LOG_EVENT_IS_QUERY(buf[EVENT_TYPE_OFFSET]) || LOG_EVENT_IS_LOAD_DATA(buf[EVENT_TYPE_OFFSET])); just to avoid someone mistakenly adding code later to call this for a wrong event type. > diff --git a/sql/sql_class.h b/sql/sql_class.h > index 2d671139a6c..05c9c4e8012 100644 > --- a/sql/sql_class.h > +++ b/sql/sql_class.h > @@ -3168,6 +3168,7 @@ class THD: public THD_count, /* this must be first */ > ulonglong start_utime, utime_after_lock, utime_after_query; > /* This can be used by handlers to send signals to the SQL level */ > ulonglong replication_flags; > + my_time_t orig_exec_time; // Exec time for last read binlog event > @@ -1348,8 +1346,14 @@ Query_log_event::Query_log_event(THD* thd_arg, const char* query_arg, > memset(&host, 0, sizeof(host)); > error_code= errcode; > > - end_time= my_time(0); > - exec_time = (ulong) (end_time - thd_arg->start_time); > + /* > + For slave threads, remember the original master exec time. > + This is needed to be able to calculate the master commit time. > + */ > + exec_time= ((thd->slave_thread) ? > + thd->orig_exec_time : > + (my_time(0) - thd_arg->start_time)); > + I think we can here avoid increasing the THD size, which is sad for a special case like this. Instead put the orig_exec_time into THD::rgi_slave, extending the struct rpl_group_info. Then the assignment above will be to rgi->orig_exec_time instead of thd->orig_exec_time, and the check here will be for thd->rgi_slave instead of thd->slave_thread. The rgi is the appropriate place for orig_exec_time, as it is a property of the transaction/event group being replicated. @@ -3171,146 +2856,170 @@ static const LEX_CSTRING msg_ignored= { STRING_WITH_LEN("Ignored") }; #endif > -static bool send_show_master_info_data(THD *thd, Master_info *mi, bool full, > - String *gtid_pos) > +inline void store_string_or_null(Field **field, const char *str) > +{ > > - mysql_mutex_unlock(&mi->run_lock); > +void store_master_info(THD *thd, Master_info *mi, TABLE *table, > > - protocol->store_string_or_null(mi->host, &my_charset_bin); > - protocol->store_string_or_null(mi->user, &my_charset_bin); > - protocol->store((uint32) mi->port); > - protocol->store((uint32) mi->connect_retry); > - protocol->store(mi->master_log_name, strlen(mi->master_log_name), > + store_string_or_null(field++, mi->host); > + store_string_or_null(field++, mi->user); > + (*field++)->store((uint32) mi->port); > + (*field++)->store((uint32) mi->connect_retry); > + (*field++)->store(mi->master_log_name, strlen(mi->master_log_name), So this patch refactors the code for SHOW SLAVE STATUS to use a shared code path with information_schema tables? That sounds like a good change. But for the future, please do such refactoring as a separate patch, you can see my patch series of pre-computing binlog checksums (MDEV-31273) for an example. Doing it together makes it very hard to review the actual code changes and see what is refactoring and what is new logic for master_slave_time_diff. > + if (unlikely(!rli->slave_timestamp) && Log_event::is_group_event(typ)) > + { > + /* > + First event for this slave. Assume that all the slave was up to date > + with the master just before the current event. > + */ > + rli->slave_timestamp= (time_t) ev->when + (time_t) ev->exec_time-1; I think the comment is meant to explain why there is the "-1" / minus-one in this expression, but it's not really clear exactly what is meant, so perhaps elaborate it a bit. Does it mean that master_slave_time_diff will start out as "1 second" just after the slave restarts? One way or the other, I do not have a strong opinion. But I want to give a general remark, from my experience with the troubles over the years with Seconds_Behind_Master. The problem with Seconds_Behind_Master was always that it didn't quite do what someone expected in some particular case, and then a new special case was added to try to accomodate. And after a number of iterations of this, we ended up with a semantics for Seconds_Behind_Master that no-one _really_ understands, and which still doesn't quite always do what people expect (which is in any case impossible if different people have different expectations). This "-1" here is clearly on the same road towards adding special cases. So my strong advice would be that, going forward, be _extremely_ conservative about modifying the semantics of master_slave_time_diff. The one good thing about the new status is that it has a (hopefully) clear semantics that can be documented (now including this "-1") and completely understood by users. Let's stick to that no matter who comes and wants just another small tweak, unless such tweaks actually make the semantics and documentation _simpler_. > + /* > + replay_log.description_event_for_exec can be null if the slave thread > + is getting killed > + */ > + if (LOG_EVENT_IS_QUERY((Log_event_type) buf[EVENT_TYPE_OFFSET]) || > + LOG_EVENT_IS_LOAD_DATA((Log_event_type) buf[EVENT_TYPE_OFFSET])) > + { > + time_t exec_time= query_event_get_time(buf, rli->relay_log. > + description_event_for_queue); > + set_if_bigger(rli->newest_master_timestamp, exec_time); > + } This confused me, because in Query_log_event, the exec_time is the duration of the query (a time interval), not an absolute time. So please use the name "end_time" for the variable instead of "exec_time", and consider renaming query_event_get_time() to query_event_get_end_time(). > > +static int make_slave_status_old_format(THD *thd, ST_SCHEMA_TABLE *schema_table) > +{ > + ST_FIELD_INFO *field_info= schema_table->fields_info; > + Name_resolution_context *context= &thd->lex->first_select_lex()->context; > + DBUG_ASSERT(thd->lex->sql_command == SQLCOM_SHOW_SLAVE_STAT); > + bool all_slaves= thd->lex->mi.show_all_slaves; > + ulonglong used_fields= ~0ULL; > + > + if (!all_slaves) > + { > + /* Remove 2 first fields and all fields above and including field 55 */ > + used_fields&= ~((1ULL << 0) | (1ULL << 1)); > + used_fields&= ((1ULL << 56)-1); Should there be some constant used for the magic numbers 55/56? And/or an assertion that this value is correct? Also, is the intention here that we will from here on never add more fields to legacy SHOW SLAVE STATUS? If so, there should probably be a comment about this; also see the test case mysql-test/suite/rpl/t/rpl_show_slave_status.test which has comments that might need updating. - Kristian

2 2

Moving forward from default-mysql-* packages in Debian
by Otto Kekäläinen 19 Jul '24

19 Jul '24

Hi! tl;dr: Anyone here interested in trying to push various open source projects in Debian and the upstreams to use mariadb-server directly instead of via default-mysql-server dependency? ## Situation Back in 2016 Debian introduced the virtual packages: - default-mysql-server - default-mysql-client - default-libmysqlclient-dev In Debian these have since pointed to MariaDB, and in Ubuntu they point to MySQL. Not much has happened since. There are only a handful packages in Debian that have declared an explicit dependency on MariaDB directly. Most packages continue to depend on default-mysql-* instead of MariaDB directly. As MariaDB and MySQL diverge, those packages will eventually end up depending on MySQL only unless they transition slowly to depend on MariaDB directly. It didn't happen organically, so it would need a push. Anyone here interested in taking up such a task? It requires talking to all the upstreams to officially support/adopt MariaDB going forward. ## Background - https://lists.debian.org/debian-devel-announce/2016/09/msg00000.html - https://lists.debian.org/debian-devel/2016/11/msg00832.html - https://wiki.debian.org/Teams/MySQL/default-mysql-server - https://tracker.debian.org/pkg/mysql-defaults ## Package lists # apt rdepends default-mysql-client default-mysql-client Reverse Depends: |Depends: audiolink |Depends: zoneminder |Depends: wordpress Recommends: tango-db |Depends: sqitch |Recommends: rsyslog-mysql |Depends: roundcube-mysql |Depends: rt5-db-mysql |Depends: rt4-db-mysql |Depends: redmine-mysql |Recommends: prelude-manager |Depends: postfixadmin |PreDepends: openstack-compute-node Depends: opendnssec-enforcer-mysql |Depends: ocsinventory-reports |Recommends: oar-server-mysql |Recommends: mysql-workbench |Suggests: mysql-sandbox Suggests: munin-plugins-core Suggests: motion Depends: libreoffice-canzeley-client Depends: lcmaps-plugins-jobrep-admin |Depends: kamailio-mysql-modules |Recommends: icinga2-ido-mysql Suggests: education-main-server |Depends: dbconfig-mysql |Suggests: cedar-backup3 |Depends: bacula-director-mysql |Depends: automysqlbackup # apt rdepends default-mysql-server default-mysql-server Reverse Depends: |Depends: python3-testing.mysqld (>= 1.0.2) |Depends: zoph |Recommends: zoneminder |Recommends: zabbix-server-mysql |Recommends: zabbix-proxy-mysql |Suggests: wordpress Suggests: trojan |Recommends: sympa |Recommends: spip |Suggests: sogo |Suggests: rsyslog-mysql Suggests: roundcube-mysql |Suggests: rt5-db-mysql |Suggests: rt4-db-mysql |Suggests: redmine-mysql |Suggests: adminer |Suggests: python3-mysqldb |Suggests: pwman3 |Suggests: prometheus-mysqld-exporter Recommends: postfix-gld |Suggests: phpmyadmin Suggests: pdns-backend-mysql Suggests: orthanc-mysql |Depends: openstack-cloud-services |Depends: openstack-cluster-installer |Suggests: ocsinventory-server |Suggests: oar-server-mysql |Recommends: mysqltuner |Suggests: mysql-sandbox |Recommends: mediawiki |Suggests: mailman3 |Suggests: mailman3-web Suggests: libreoffice-canzeley-client |Suggests: libreoffice-sdbc-mysql Suggests: inspircd Suggests: icingadb |Suggests: icinga2-ido-mysql Suggests: goval-dictionary Suggests: golang-github-ctdk-goiardi-dev Suggests: goiardi Suggests: digitemp |Depends: diaspora-common |Suggests: dbconfig-mysql Suggests: collectd-core |Recommends: cacti |Recommends: bacula-director-mysql |Recommends: audiolink # apt rdepends default-libmysqlclient-dev default-libmysqlclient-dev Reverse Depends: Depends: libboinc-app-dev Depends: libvtk9-dev Depends: urweb Depends: librust-mysqlclient-sys-dev Suggests: qtbase5-gles-dev Suggests: qtbase5-dev |Depends: postgresql-16-mysql-fdw Depends: libpoco-dev Recommends: newlisp |Suggests: mysql-sandbox Depends: libmysql-ocaml-dev Depends: libmysql++-dev Depends: libmailutils-dev Depends: kannel-dev Depends: libhoel-dev Suggests: libglpk40 Depends: libgdal-dev Suggests: fp-units-db-3.2.2 Suggests: fp-units-win-db-3.2.2 Depends: cl-sql-mysql

1 0

Re: [MariaDB commits] [PATCH] MDEV-15393 part 2. Fixes to 'gtid_slave_pos duplicate key errors after mysqldump restore' MDEV-34615 mysqldump wipes off pre-existing gtid slave state
by Kristian Nielsen 18 Jul '24

18 Jul '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> The most common case is the provisioning of a slave with a complete dump >> from a master. In this case, it is required that the GTID position is set >> correctly across all domains. Merging the existing and new GTID position as >> attempted in this patch will break the provisioning of the slave. > > 'Will break' but merely for the technical (your pp1,2 to the concat) > reasons. I don't see what else can be a problem of MDEV-34615 part. No. What I mean is, consider some slave server that has an existing gtid_slave_pos='20-2-1000' say, domain_id=20. And then this server is for some reason re-provisioned (ie. all data overwritten) with a new mysqldump, say from a master with GTID pos '10-1-2000'. Now the correct GTID position matching the new dump is SET GLOBAL gtid_slave_pos='10-1-2000'; If you make the MDEV-34615 idea default, then the position for the new slave will be multi-domain '20-2-1000,10-1-2000', which is not correct and could cause problems later. Basically, only the user knows if the old @@gtid_slave_pos is meaningful and should be kept, or if it should be overwritten, mysqldump cannot reliably determine this. And changing the default will end up breaking things in some cases that are working now. The merging of GTID positions can be useful in the case where we are setting up a multi-source slave C by importing dumps from two master A and B, say. In this case the first import needs to overwrite any old gtid_slave_pos, and the second import should merge. Thus the need for the user to specify. I hope this explains the problem. - Kristian.

1 0

Re: [MariaDB commits] [PATCH] MDEV-15393 part 2. Fixes to 'gtid_slave_pos duplicate key errors after mysqldump restore' MDEV-34615 mysqldump wipes off pre-existing gtid slave state
by Kristian Nielsen 17 Jul '24

17 Jul '24

> commit 7a50135f3f908380e2dfb45e3f95c9fb10c01d62 > Author: Andrei <andrei.elkin(a)mariadb.com> > Date: Mon Jul 15 17:50:37 2024 +0300 > > MDEV-15393 part 2. Fixes to 'gtid_slave_pos duplicate key errors after mysqldump restore' > MDEV-34615 mysqldump wipes off pre-existing gtid slave state Hi Andrei, here my review of the patch, with a particular important issue to not break existing use cases and backwards compatibility, as well as some general issues/comments. > When mysqldump run on mysql system database it generates inserts sql commands > into mysql.gtid_slave_pos. After running the backup script those Here and below, I suggest better wording for text that is hard to understand or using wrong/unusual grammar. "When mysqldump is run to dump the `mysql` system database, it generates INSERT statements into the table mysql.gtid_slave_pos" > inserts did not produce an expected gtid state on slave. "did not produce the exspected gtid state" > In particular the maximum of mysql.gtid_slave_pos.sub_id did not make > into > rpl_global_gtid_slave_state.last_sub_id > a memory object that the table persists. I think you mean here "an in-memory object that is supposed to match the current state of the table". > And that was regardless of whether --gtid option in or out. "whether the --gtid option was specified by the user or not". > The fixes ensure the insert block of the dump script is followed > with a "summing-up" SET @global.gtid_slave_pos assignment. > To address MDEV-34615 the assignment makes sure a possibly > pre-existing gtid state (e.g reflecting another domain) remains as a > sub-state upon the dump installation. This change is not appropriate, as it breaks backwards compatibility and breaks a common use case. Certainly not in a GA release such as 10.5! The most common case is the provisioning of a slave with a complete dump from a master. In this case, it is required that the GTID position is set correctly across all domains. Merging the existing and new GTID position as attempted in this patch will break the provisioning of the slave. The existing supported way of not overwriting the slave's old @@GLOBAL.gtid_slave_pos is to use --master-data=2 --gtid. This will output the GTID position in the dump commented out, and the user can themselves (manually or through a script of their own) merge the positions as required for their setup and application. If you think it is important to have a facility in mysqldump to do merging automatically, then you should implement this as a new option. Such new option would need to go to the next development version (11.6 or 11.7 I suppose?). It could be --master-data = 5 or 6 for example (idea being bit 2 means "merge old and new position), or maybe some other way that doesn't require "magic" constants. I am commenting below on some problems with the implementation of the merging of old and new position. But note that these comments apply to a different patch that implement this as a new option. These changes must not be made to the existing default behaviour. > diff --git a/client/mysqldump.c b/client/mysqldump.c > index 1c9d92d6e09..0be890e2528 100644 > --- a/client/mysqldump.c > +++ b/client/mysqldump.c > @@ -5985,8 +5985,13 @@ static int dump_selected_tables(char *db, char **table_names, int tables) > } /* dump_selected_tables */ > > > +const char fmt_gtid_pos[] = "%sSET GLOBAL gtid_slave_pos=concat('%s', " > + "if(length(@@global.gtid_slave_pos) = 0, '', ','), " > + "@@global.gtid_slave_pos);\n"; Two problems with this: 1. This will result in a syntax error if the new position is empty and the old position is not (eg. SET GLOBAL gtid_slave_pos=',0-1-2'). 2. Unless I'm missing something obvious, this doesn't actually work to merge two different GTIDs with the same domain! It produces an error ER_DUPLICATE_GTID_DOMAIN. It seems there is a lack of coverage in the associated test case, if the actual merging of domains isn't even tested? SELECT @@GLOBAL.gtid_slave_pos; @@GLOBAL.gtid_slave_pos 0-1-2 SET GLOBAL gtid_slave_pos= CONCAT('0-1-10', ',', @@GLOBAL.gtid_slave_pos); rpl.tmp2 'mix' [ fail ] mysqltest: At line 12: query 'SET GLOBAL gtid_slave_pos= CONCAT('0-1-10', ',', @@GLOBAL.gtid_slave_pos)' failed: ER_DUPLICATE_GTID_DOMAIN (1943): GTID 0-1-2 and 0-1-10 conflict (duplicate domain id 0) > @@ -6054,8 +6059,11 @@ static int do_show_master_status(MYSQL *mysql_con, int consistent_binlog_pos, > fprintf(md_result_file, > "%sCHANGE MASTER TO MASTER_USE_GTID=slave_pos;\n", > comment_prefix); > - fprintf(md_result_file, > - "%sSET GLOBAL gtid_slave_pos='%s';\n", > + /* > + Defer print of SET gtid_slave_pos until after its placeholder > + table is guaranteed to have been dumped. > + */ > + sprintf(set_gtid_pos, fmt_gtid_pos, set_gtid_pos is a fixed size buffer, if I understand the patch correctly. This seems to mean that you have a buffer overflow here if the GTID position has too many domains? Note that it is not appropriate to simply use snprintf() instead, as this will merely truncate the output and give silent data corruption (or in this case probably a strange syntax error). Instead at least a proper error should be given (better to support arbitrary GTID position of course, but that might be a separate task if this is already an existing limitation of mysqldump, I did not check). > @@ -6181,10 +6189,8 @@ static int do_show_slave_status(MYSQL *mysql_con, int use_gtid, > mysql_free_result(slave); > return 1; > } > - print_comment(md_result_file, 0, > - "-- GTID position to start replication:\n"); > - fprintf(md_result_file, "%sSET GLOBAL gtid_slave_pos='%s';\n", > - gtid_comment_prefix, gtid_pos); > + /* similarly to do_show_master_status */ > + sprintf(set_gtid_pos, fmt_gtid_pos, gtid_comment_prefix, gtid_pos); I assume this is also a potential buffer overflow, didn't check closely. > @@ -6913,6 +6932,11 @@ int main(int argc, char **argv) > int exit_code; > int consistent_binlog_pos= 0; > int have_mariadb_gtid= 0; > + /* > + to hold SET @@global.gtid_slave_pos which is deferred to print > + until the function epilogue. > + */ > + char set_gtid_pos[3 + sizeof(fmt_gtid_pos) + MAX_GTID_LENGTH]= {0}; - Kristian.

1 0

'_' in numeric literals [MDEV-33228]
by Rohan Gupta 16 Jul '24

16 Jul '24

I was going through this issue https://jira.mariadb.org/browse/MDEV-34228, and noticed that during parsing we check if '_' is present in a token , if it does it is parsed as an identifier. My question here is to follow a SQL2023 standard of '_' in numeric literals, how would one go distinguishing the difference between numeric literals and identifiers ? mariadb documentation says that a identifier cannot start with a number until its in backticks `{identifier}`. Rohan Gupta

2 1

Re: d78376da69d: MDEV-34504 PURGE BINARY LOGS not working anymore
by Sergei Golubchik 10 Jul '24

10 Jul '24

Hi, Monty, On Jul 09, Michael Widenius wrote: > revision-id: d78376da69d (mariadb-11.4.2-11-gd78376da69d) > parent(s): 4ffeca643d5 > author: Michael Widenius > committer: Michael Widenius > timestamp: 2024-07-09 11:01:29 +0300 > message: > > MDEV-34504 PURGE BINARY LOGS not working anymore > > PURGE BINARY LOGS did not always purge binary logs. This commit fixes > some of the issues and adds notifications if a binary log cannot be > purged. > > User visible changes: > - 'PURGE BINARY LOG TO log_name' and 'PURGE BINARY LOGS BEFORE date' > worked differently. 'TO' ignored 'slave_connections_needed_for_purge' > while 'BEFORE' did not. Now both versions ignores the > 'slave_connections_needed_for_purge variable'. > - 'PURGE BINARY LOG..' commands now returns 'note' if a binary log cannot > be deleted like > Note 1375 Binary log 'master-bin.000004' is not purged because it is > the current active binlog > - Automatic binary log purges, based on date or size, will write a > note to the error log if a binary log matching the size or date > cannot yet be deleted. > - If 'slave_connections_needed_for_purge' is set from a config or > command line, it is set to 0 if Galera is enabled and 1 otherwise > (old default). This ensures that automatic binary log purge works > with Galera as before the addition of > 'slave_connections_needed_for_purge'. > If the variable is changed to 0, a warning will be printed to the error > log. > > Code changes: > - Added THD argument to several purge_logs related functions that needed > THD. > - Added 'interactive' options to purge_logs functions. This allowed > me to remove testing of sql_command == SQLCOM_PURGE. it's not really _ineractive_ but ok > - Changed purge_logs_before_date() to first check if log is applicable > before calling can_purge_logs(). This ensures we do not get a > notification for logs that does not match the remove criteria. > - MYSQL_BIN_LOG::can_purge_log() will write notifications to the user > or error log if a log cannot yet be removed. > - log_in_use() will return reason why a binary log cannot be removed. > - Moved checking of binlog_format for Galera to be after Galera is > initialized (The old check never worked). If Galera is enabled > we now change the binlog_format to ROW, with a warning, instead of > aborting the server. If this change happens, the binlog_format variable > will be marked with AUTO or FORCED, for information_schema.system_variables, > and a warning will be printed to the error log. What's "FORCE"? "AUTO" means that the value was automatically changed by the server, "FORCE" means the same, it looks redundant. And it's not the "source of the value", better to revert that part. > - Print also a warning if FLASHBACK changes the binlog_format to ROW. > Before this was done silently. > > diff --git a/sql/mysqld.cc b/sql/mysqld.cc > index 2b12001de9e..604eaadcc92 100644 > --- a/sql/mysqld.cc > +++ b/sql/mysqld.cc > @@ -5847,7 +5850,28 @@ int mysqld_main(int argc, char **argv) > #ifdef WITH_WSREP > wsrep_set_wsrep_on(nullptr); > if (WSREP_ON && wsrep_check_opts()) unireg_abort(1); > -#endif > + > + if (!opt_bootstrap && WSREP_PROVIDER_EXISTS && WSREP_ON) > + { > + if (global_system_variables.binlog_format != BINLOG_FORMAT_ROW) > + { > + sql_print_warning("Binlog_format changed to \"ROW\" because of Galera"); should be sql_print_information() > + global_system_variables.binlog_format= BINLOG_FORMAT_ROW; > + mark_binlog_format_used(binlog_format_used); > + } > + binlog_format_used= 1; > + if (!slave_connections_needed_for_purge_option_used) > + { > + slave_connections_needed_for_purge= > + internal_slave_connections_needed_for_purge= 0; > + mark_slave_connections_needed_for_purge_as_auto(); you don't need that, you can use SYSVAR_AUTOSIZE(internal_slave_connections_needed_for_purge, 0); > + sql_print_information( > + "slave_connections_needed_for_purge changed to 0 because " > + "of Galera. Change it to 1 or higher if this Galera node " > + "is also Master in a normal replication setup"); > + } > + } > +#endif /* WITH_WSREP */ > > #ifdef _WIN32 > /* > @@ -8219,7 +8243,9 @@ mysqld_get_one_option(const struct my_option *opt, const char *argument, > ((enum_slave_parallel_mode)opt_slave_parallel_mode); > break; > } > - > + case (int) OPT_SLAVE_CONNECTIONS_NEEDED_FOR_PURGE: > + slave_connections_needed_for_purge_option_used= 1; you don't need that, you can use if (!IS_SYSVAR_AUTOSIZE(&internal_slave_connections_needed_for_purge)) > + break; > case (int)OPT_BINLOG_IGNORE_DB: > { > binlog_filter->add_ignore_db(argument); > @@ -8714,18 +8740,14 @@ static int get_options(int *argc_ptr, char ***argv_ptr) > opt_bin_log= opt_bin_log_used= 1; > > /* Force format to row */ > + if (global_system_variables.binlog_format != BINLOG_FORMAT_ROW) > + { > + sql_print_warning("Binlog_format changed to \"ROW\" because of " > + "flashback"); sql_print_information > + global_system_variables.binlog_format= BINLOG_FORMAT_ROW; > + mark_binlog_format_used(binlog_format_used); > + } > binlog_format_used= 1; > - global_system_variables.binlog_format= BINLOG_FORMAT_ROW; > - } > - > - if (!opt_bootstrap && WSREP_PROVIDER_EXISTS && WSREP_ON && > - global_system_variables.binlog_format != BINLOG_FORMAT_ROW) > - { > - > - WSREP_ERROR ("Only binlog_format = 'ROW' is currently supported. " > - "Configured value: '%s'. Please adjust your configuration.", > - binlog_format_names[global_system_variables.binlog_format]); > - return 1; > } Regards, Sergei Chief Architect, MariaDB Server and security(a)mariadb.org

2 1

GitHub account owners - please +1 these MariaDB mention PRs
by Otto Kekäläinen 10 Jul '24

10 Jul '24

Hi! I assume everyone on this mailing list has a GitHub account. Could you please +1 these PRs that add a mention of MariaDB in the documentation of various projects that currently only mention MySQL? Open each link below and press the thumbs up icon in the lower left corner of the PR description: https://github.com/bookshelf/bookshelf/pull/2129 https://github.com/ponyorm/pony/pull/708 https://github.com/metabase/metabase/issues/40325 https://github.com/SeaQL/seaql.github.io/pull/116 I am asking this because there are a lot of database tools and libraries out there that work with both MySQL and MariaDB, but they only mention MySQL, which is a pity, because the users of those tools might actually switch from MariaDB to MySQL simply out of confusion/fear that the tool supports only MySQL, which is almost never true. One of those rare cases is Javascript/Node.js Drizzle ORM which needs changes to support MariaDB. There is already a PR for that, you could +1 it as well: https://github.com/drizzle-team/drizzle-orm/pull/1692 The Rust Diesel ORM has a similar situation going on: https://github.com/diesel-rs/diesel/issues/1882 https://github.com/diesel-rs/diesel/pull/3964 However most projects have absolutely no MySQL-specific things and work with MariaDB, and just needed the docs updated, like was done in these cases: https://github.com/mysql-net/MySqlConnector/pull/1460 https://github.com/rails/rails/pull/51330 https://github.com/prisma/docs/pull/5706 https://github.com/gregstoll/gallery2/pull/168 https://github.com/coleifer/peewee/pull/2858

3 5