- developers - lists.mariadb.org

3 PRs pending second approver
by Otto Kekäläinen 25 Apr '24

25 Apr '24

Hi all with permission to review / add approval, This is ready to be merged into 11.5 and already has approval from Andrew, just needs one more approval: https://github.com/MariaDB/server/pull/2671 This has also one approval from Andrew, pending a second to be merged: https://github.com/MariaDB/server/pull/2169 Likewise, approved by Andrew, pending second: https://github.com/MariaDB/server/pull/2516 Additionally, this has been open for since January without any reviews: https://github.com/MariaDB/server/pull/2985 This was updated a month ago based on feedback, but hasn't gotten a re-review since: https://github.com/MariaDB/server/pull/3114 Pending re-review: https://github.com/MariaDB/server/pull/2556 - Otto

3 2

Re: [MariaDB commits] [PATCH] MDEV-33602: Sporadic test failure in rpl.rpl_gtid_stop_start
by Kristian Nielsen 25 Apr '24

25 Apr '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >>> --- a/sql/rpl_rli.cc >>> +++ b/sql/rpl_rli.cc >>> @@ -1030,7 +1030,7 @@ void Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos, >>> rgi->last_master_timestamp > last_master_timestamp) >>> last_master_timestamp= rgi->last_master_timestamp; >>> } >>> - else >>> + else if (!mi->using_gtid || seen_gtid_log_list_event) >>> { >>> /* Non-parallel case. */ >>> group_relay_log_pos= event_relay_log_pos; >> >> Agree, this looks fine. >> The dump thread should always be sending the gtid_list event. And yes, the >> log_pos of the events received until then are not useful to set for the >> slave. >> >> Right, so this seems a good approach for a fix. >> >> Maybe I should push the workaround to 10.5 to help the effort to remove >> sporadic failures, and then file a separate bug about the underlying problem >> and your proposed solution? > > Sounds reasonable. Thank you! Filed as https://jira.mariadb.org/browse/MDEV-33996 - Kristian.

1 0

Re: [MariaDB commits] [PATCH] MDEV-33602: Sporadic test failure in rpl.rpl_gtid_stop_start
by Kristian Nielsen 24 Apr '24

24 Apr '24

Andrei Elkin <andrei.elkin(a)mariadb.com> writes: >> 1. I have not so far been able to reproduce the failure, so this patch is >> assumed to fix the sporadic failure, but I have not been able to verify that >> it does. > > I verified your analysis to confirm. Thanks for checking! >> But on the other hand this will not solve the fundamental issue, that until the >> GTID mode connect is completed, we will in the general case not have a valid >> corresponding old-style position. So maybe the current behaviour is >> ok? > Sure but it looks fixes are not overly difficult. > > As it's Gtid_list_log_event::log_pos that makes the file:pos (old-style coordinates) > state be valid why won't be keep the coordinates intact until the event > has been arrived/processed? Say that situation is remembered in `RLI::seen_gtid_log_list_event`. > Then for instance in the serial case the fixes would look like > > --- a/sql/rpl_rli.cc > +++ b/sql/rpl_rli.cc > @@ -1030,7 +1030,7 @@ void Relay_log_info::inc_group_relay_log_pos(ulonglong log_pos, > rgi->last_master_timestamp > last_master_timestamp) > last_master_timestamp= rgi->last_master_timestamp; > } > - else > + else if (!mi->using_gtid || seen_gtid_log_list_event) > { > /* Non-parallel case. */ > group_relay_log_pos= event_relay_log_pos; Agree, this looks fine. The dump thread should always be sending the gtid_list event. And yes, the log_pos of the events received until then are not useful to set for the slave. Right, so this seems a good approach for a fix. Maybe I should push the workaround to 10.5 to help the effort to remove sporadic failures, and then file a separate bug about the underlying problem and your proposed solution? Thanks, - Kristian.

1 0

Re: [MariaDB commits] [PATCH] MDEV-33602: Sporadic test failure in rpl.rpl_gtid_stop_start
by Kristian Nielsen 23 Apr '24

23 Apr '24

Hi Andrei, Brandon, What do you think of this patch to fix sporadic failure of rpl.rpl_gtid_stop_start in buildbot, MDEV-33602? The test fails relatively frequent, so would be good to get it fixed. Two issues: 1. I have not so far been able to reproduce the failure, so this patch is assumed to fix the sporadic failure, but I have not been able to verify that it does. 2. The question is if this will be just hiding a real bug? When the GTID-mode slave connects, it receives a few events from the start of the binlog (Gtid_list, Binlog_checkpoint, etc), and these briefly update the old-style position to a wrong value until the fake Gtid_list is received with the correct position. One might argue that this should be avoided. But on the other hand this will not solve the fundamental issue, that until the GTID mode connect is completed, we will in the general case not have a valid corresponding old-style position. So maybe the current behaviour is ok? - Kristian. Kristian Nielsen via commits <commits(a)lists.mariadb.org> writes: > The test could fail with a duplicate key error because switching to non-GTID > mode could start at the wrong old-style position. The position could be > wrong when the previous GTID connect was stopped before receiving the fake > GTID list event which gives the old-style position corresponding to the GTID > connected position. > > Work-around by injecting an extra event and syncing the slave before > switching to non-GTID mode. > > Signed-off-by: Kristian Nielsen <knielsen(a)knielsen-hq.org> > --- > .../suite/rpl/r/rpl_gtid_stop_start.result | 4 ++++ > mysql-test/suite/rpl/t/rpl_gtid_stop_start.test | 16 ++++++++++++++++ > 2 files changed, 20 insertions(+) > > diff --git a/mysql-test/suite/rpl/r/rpl_gtid_stop_start.result b/mysql-test/suite/rpl/r/rpl_gtid_stop_start.result > index ae0050c353a..e8633cd45bb 100644 > --- a/mysql-test/suite/rpl/r/rpl_gtid_stop_start.result > +++ b/mysql-test/suite/rpl/r/rpl_gtid_stop_start.result > @@ -111,6 +111,10 @@ a > 6 > 7 > *** MDEV-4486: Allow to start old-style replication even if mysql.gtid_slave_pos is unavailable > +connection server_1; > +INSERT INTO t1 VALUES (8); > +DELETE FROM t1 WHERE a=8; > +connection server_2; > connection server_2; > include/stop_slave.inc > CHANGE MASTER TO master_use_gtid= no; > diff --git a/mysql-test/suite/rpl/t/rpl_gtid_stop_start.test b/mysql-test/suite/rpl/t/rpl_gtid_stop_start.test > index b5ff294908b..032ebb77d1e 100644 > --- a/mysql-test/suite/rpl/t/rpl_gtid_stop_start.test > +++ b/mysql-test/suite/rpl/t/rpl_gtid_stop_start.test > @@ -173,6 +173,22 @@ SELECT * FROM t1 ORDER BY a; > > --echo *** MDEV-4486: Allow to start old-style replication even if mysql.gtid_slave_pos is unavailable > > +# In GTID mode, the old-style replication position is also updated. But during > +# GTID connect, the old-style position is not known until receiving the fake > +# GTID list event, which contains the required position value. If we happened > +# to stop the slave above before this fake GTID list event, the test could fail > +# with duplicate key errors due to switching to non-GTID mode at a wrong > +# position too far back in the binlog. > +# > +# Work-around this by injecting an extra dummt event and syncing the slave to > +# it, ensuring the old-style position will be updated. > +--connection server_1 > +INSERT INTO t1 VALUES (8); > +DELETE FROM t1 WHERE a=8; > +--save_master_pos > +--connection server_2 > +--sync_with_master > + > --connection server_2 > --source include/stop_slave.inc > CHANGE MASTER TO master_use_gtid= no;

1 0

MDEV-27512, --slave-skip-errors, and optimistic parallel replication
by Kristian Nielsen 18 Apr '24

18 Apr '24

Hi Brandon, I only now got to check on MDEV-27512. This bug seems to be about using --slave_skip_errors=ALL with optimistic parallel replication. I saw your patch for the bug. While the patch by itself is perhaps ok, I think it's only fixing a minor thing, and not addressing the real problem, let me explain my thinking: Now, --slave-skip-errors is obviously a dangerous option, but presumably the use case is to make the slave continue at all costs in case an unexpected error occurs, rather than stop the slave and break replication. But optimistic parallel replication by design can introduce many different transient errors due to conflicts, that are then handled by rolling back and retrying the offending transactions. These errors are *expected*, and they do *not* cause slave to stop or replication to break. However, it appears that --slave_skip_errors also affects such transient errors due to optimistic parallel replication, and will cause any such transaction to be silently ignored! This must be very wrong, it will cause massive replication divergence. It seems to me this is the real bug here. When an error is encountered during optimistic apply of a transaction (is_parallel_retry_error() returns true, eg. rgi->speculation == SPECULATE_OPTIMISTIC, then this error should _not_ be subject to --slave-skip-errors. The transaction should be rolled back as normal, and wait_for_prior_commit() done. Then after setting rgi->speculation = SPECULATE_WAIT and retrying, if we still get the error, --slave-skip-errors can apply. I put together a quick test that seems to show this behaviour, included below. This tests replicates correctly without replication stopping with error. But running it with --mysqld=--slave-skip-errors=all, it replicates incorrectly, skipping lots of transactions. The test is somewhat contrieved, but I think it shows the real problem, that --slave-skip-errors can randomly cause transactions to be skipped or not depending on if optimistic parallel replication triggers a matching transient error or not. So in summary, it looks like there is a real problem here, that optimistic parallel replication is not working correctly with --slave-skip-errors, transient errors incorrectly causes conflicts to skip transactions rather than retrying them. This will cause replication to diverge even when no real errors occur. And the patch for MDEV-27512 doesn't seem to address this issue. So I think MDEV-27512 should be re-opened, what do you think? - Kristian. ----------------------------------------------------------------------- --source include/have_innodb.inc --source include/have_binlog_format_row.inc --source include/master-slave.inc --connection master ALTER TABLE mysql.gtid_slave_pos ENGINE=InnoDB; CREATE TABLE t1 (a INT PRIMARY KEY, b INT) ENGINE=InnoDB; INSERT INTO t1 VALUES (1,NULL), (2,2), (3,NULL), (4,4), (5, NULL), (6, 6); --sync_slave_with_master --source include/stop_slave.inc CHANGE MASTER TO master_use_gtid=slave_pos; SET @old_timeout= @@GLOBAL.innodb_lock_wait_timeout; SET GLOBAL innodb_lock_wait_timeout= 5; SET @old_parallel= @@GLOBAL.slave_parallel_threads; SET @old_mode= @@GLOBAL.slave_parallel_mode; SET GLOBAL slave_parallel_mode= aggressive; SET GLOBAL slave_parallel_threads= 20; --connection master UPDATE t1 SET b=b+1 WHERE a=6; --disable_query_log let $i= 0; while ($i < 40) { eval UPDATE t1 SET b=b+1 WHERE a=2; inc $i; } --enable_query_log SELECT * FROM t1 ORDER BY a; --save_master_pos --connection slave1 # Block first worker, and recursively pause all following workers that get # temporary errors before they can retry. BEGIN; SELECT * FROM t1 WHERE a=6 FOR UPDATE; --connection slave # Cause initial row not found error. SET STATEMENT sql_log_bin=0 FOR UPDATE t1 SET a=7 WHERE a=2; --source include/start_slave.inc --sleep 2 # Now following workers should be waiting for prior commit before retrying. # Remove the row not found error. SET STATEMENT sql_log_bin=0 FOR UPDATE t1 SET a=2 WHERE a=7; --connection slave1 ROLLBACK; --connection slave --sync_with_master SELECT * FROM t1 ORDER BY a; # Cleanup --connection slave --source include/stop_slave.inc SET GLOBAL innodb_lock_wait_timeout= @old_timeout; SET GLOBAL slave_parallel_threads= @old_parallel; SET GLOBAL slave_parallel_mode= @old_mode; --source include/start_slave.inc --connection default DROP TABLE t1; --source include/rpl_end.inc

2 2

Feedback plugin status?
by Otto Kekäläinen 16 Apr '24

16 Apr '24

Hi! I was about to merge a long overdue MR in Debian to enable the Feedback plugin (https://salsa.debian.org/mariadb-team/mariadb-server/-/merge_requests/16) However I noticed all the links at https://mariadb.org/about/#usage-statistics are broken. The wiki page at https://mariadb.com/kb/en/feedback-plugin/ says that the plugin is 'currently not working'. What does this mean? What is the path to get it working again? Is there a need for somebody to rewrite the stats server part? Thanks, - Otto

3 4

MariaDB/MySQL wire protocol ownership and MariaDB Connector C source code hosting?
by Otto Kekäläinen 13 Apr '24

13 Apr '24

Hi! The MariaDB wire protocol is an interplay between the MariaDB Server and the MariaDB Connector C. However, only the server is in the MariaDB Foundation's GitHub account at https://github.com/mariadb/server, and the Connector C is in the MariaDB PLC account at https://github.com/mariadb-corporation/mariadb-connector-c. Is there perhaps some plan to hand over the Connector C to the MariaDB Foundation? The reason I am asking is that the connector does not seem equally "true open source" as the server is, making changes that require coordination in both server and connector (e.g. https://github.com/mariadb-corporation/mariadb-connector-c/pull/227) kind of out-of-scope for the MariaDB Foundation, which seems counterproductive. If/when wire protocol changes might be made and changes to both server and libmariadb coordinated in an open "working group" like suggested in https://github.com/MariaDB/server/pull/2684, it might be very challenging for the Foundation or the open source community to drive it if the PLC staff member maintaining the connector does not participate. Also, seems the PR reviews at https://github.com/mariadb-corporation/mariadb-connector-c/pulls have been stalled since 2022. MariaDB Foundation staff are funded to facilitate reviews, but can't really be *responsible* for reviewing them as long as it is not a Foundation repository and review policies are not owned by the Foundation. It would just make a lot of sense if such a central piece of the MariaDB Server would be in the Foundation's GitHub account. The MariaDB Connector C / libmaraidb is already published and distributed by the MariadB Foundation, so moving the source code hosting over shouldn't big such a big thing, right? - Otto

3 2

7 PRs pending second approver - can somebody do it?
by Otto Kekäläinen 11 Apr '24

11 Apr '24

Hi Sergei, Andrew or anyone else who wants to help, These PRs have already been approved by one reviewer 2-8 weeks ago and are pending the second reviewer to approve/merge. These indicate that Sergei's review is requested, but I guess anybody could be the second reviewer: https://github.com/MariaDB/server/pull/2169 https://github.com/MariaDB/server/pull/2671 The indicate that Andrewi's review is requested, but I guess anybody could be the second reviewer: https://github.com/MariaDB/server/pull/3100 https://github.com/MariaDB/server/pull/3120 https://github.com/MariaDB/server/pull/3125 https://github.com/MariaDB/server/pull/3142 https://github.com/MariaDB/server/pull/3154 I am sending this to the mailing list as well just to raise general awareness that while Andrew and Daniel are putting in a lot of effort to review "external" contributions, it is not enough and even approved PRs get stalled. Other core contributors should perhaps get into the habit of reviewing/approving PRs as well. If there was a rule in github.com/MariaDB/server that prevents any commits from being merged unless it has two approvals, it would both help improve the code quality AND also force core contributors to spend more time adding approvals on GitHub and thus automatically increase the likelihood that there are no one-approval PRs around for weeks and weeks without getting the second approval. - Otto

3 2

MariaDB supports DBdeployer and MySqlJS, but they don't support MariaDB?
by Otto Kekäläinen 09 Apr '24

09 Apr '24

Hi! While digging into the CI situation I noticed that https://buildbot.mariadb.org/ advertises: "We run other ecosystem tests for: * DBdeployer * MySqlJS" However, when I browse https://github.com/mysqljs/mysql and https://github.com/datacharmer/dbdeployer there are zero mentions of MariaDB. Just wondering is anybody working to get them to mention MariaDB in their docs? If they are tested as part of the MariaDB CI, surely they should reciprocate and at least tell in their README that MariaDB is also supported. Maybe somebody from the MariaDB Foundation could send a PR to update their README, so they don't need to do anything else than just accept it. This was done for example in https://github.com/mysql-net/MySqlConnector/pull/1460/files by Daniel Lenski, and now that project mentions both MySQL and MariaDB. - Otto

3 2

Safe use of sprint/printf/fprintf in MariaDB code base
by Otto Kekäläinen 08 Apr '24

08 Apr '24

Hi all, Reading about the xz-utils backdoor authors submission of converting safe_fprintf() to fprintf() in libarchive[1] presumably in order to introduce intentional vulnerability reminded me that the MariaDB code base still has a plenty unsafe sprint/printf/fprintf use that can easily be found with scanners such as Flawfinder[2] and cppcheck[3]. There are currently 6 merge requests open by two authors (CC'd) to fix some of these issues[4]. Could we please have some more attention on these by the core contributors? If core contributors are not happy with the submissions, could you perhaps write your own safe functions (there are already some in m_string.h[5]) like many other projects seem to have (also libarchive had[6]) and then ask all contributors to use them consistently? Use of specific memory safe functions could also be mandated via the coding standards[7]. [1] https://github.com/libarchive/libarchive/pull/1609 [2] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L461-L489 [3] https://github.com/MariaDB/server/blob/11.5/.gitlab-ci.yml#L522-L554 [4] https://github.com/MariaDB/server/pulls?q=is%3Apr+is%3Aopen+sprintf+ [5] https://github.com/MariaDB/server/blob/11.5/include/m_string.h [6] https://github.com/libarchive/libarchive/blob/6110e9c82d8ba830c3440f36b9904… [7] https://github.com/MariaDB/server/blob/11.5/CODING_STANDARDS.md

2 3