- developers - lists.mariadb.org

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

MariaDB 11.4 build consumers too much disk space - solved by not shipping the embedded server in Debian anymore
by Otto Kekäläinen 10 Jul '24

10 Jul '24

Hi all! While preparing 11.4.2 for upload to Debian[1] I noticed that builders fail on lack of disk space[2]. Below are some listings of build artifacts sorted by size. I solved this for now by stopping[3] to ship the embedded server in Debian, I don't think it had many users anyway. Leaving this here for visibility/comments though. [1] https://salsa.debian.org/mariadb-team/mariadb-server/-/merge_requests/88 [2] https://salsa.debian.org/otto/mariadb-server/-/jobs/5793989 [3] https://salsa.debian.org/mariadb-team/mariadb-server/-/merge_requests/88/di… ± du -shc * | sort -hr 12G total 7,8G builddir 3,7G debian 234M mysql-test 95M storage 30M sql 17M strings 5,1M plugin ± find debian/tmp/ -type f -exec du -b {} \; | sort -n | tail -n 25 8938376 debian/tmp/usr/bin/myisampack 8956744 debian/tmp/usr/bin/mariadb-test 9441288 debian/tmp/usr/bin/myisamchk 9661400 debian/tmp/usr/bin/mariadb-client-test 9725048 debian/tmp/usr/bin/mariadb-binlog 9894072 debian/tmp/usr/lib/mysql/plugin/ha_spider.so 10277896 debian/tmp/usr/lib/mysql/plugin/ha_connect.so 10622344 debian/tmp/usr/bin/aria_s3_copy 11875712 debian/tmp/usr/bin/aria_dump_log 12048792 debian/tmp/usr/bin/aria_ftdump 12079744 debian/tmp/usr/bin/aria_pack 12814096 debian/tmp/usr/bin/aria_read_log 12943168 debian/tmp/usr/bin/aria_chk 25659064 debian/tmp/usr/lib/mysql/plugin/ha_mroonga.so 138399064 debian/tmp/usr/bin/sst_dump 143275720 debian/tmp/usr/lib/mysql/plugin/ha_rocksdb.so 145093496 debian/tmp/usr/bin/mariadb-ldb 214442664 debian/tmp/usr/bin/test-connect-t 214822248 debian/tmp/usr/bin/mariadb-embedded 214849432 debian/tmp/usr/lib/x86_64-linux-gnu/libmariadbd.so.19 215011504 debian/tmp/usr/bin/mariadb-test-embedded 216148632 debian/tmp/usr/bin/mariadb-client-test-embedded 248523904 debian/tmp/usr/sbin/mariadbd 256329048 debian/tmp/usr/bin/mariadb-backup 622299760 debian/tmp/usr/lib/x86_64-linux-gnu/libmariadbd.a ± find builddir -type f -exec du -b {} \; | sort -n | tail -n 25 18278408 builddir/storage/rocksdb/librocksdb_tools.a 25227146 builddir/sql/libwsrep.a 25298746 builddir/sql/libwsrep_provider.a 25614608 builddir/storage/mroonga/vendor/groonga/lib/libgroonga.a 25659064 builddir/storage/mroonga/ha_mroonga.so 92889078 builddir/storage/perfschema/libperfschema_embedded.a 97604058 builddir/storage/perfschema/libperfschema.a 125637396 builddir/storage/innobase/libinnobase_embedded.a 131904864 builddir/storage/innobase/libinnobase.a 138399064 builddir/storage/rocksdb/sst_dump 143275720 builddir/storage/rocksdb/ha_rocksdb.so 145093496 builddir/storage/rocksdb/mariadb-ldb 189426944 builddir/unittest/sql/my_json_writer-t 189648448 builddir/unittest/sql/explain_filename-t 214442664 builddir/unittest/embedded/test-connect-t 214822248 builddir/libmysqld/examples/mariadb-embedded 214849432 builddir/libmysqld/libmariadbd.so.19 215011504 builddir/libmysqld/examples/mariadb-test-embedded 216148632 builddir/libmysqld/examples/mariadb-client-test-embedded 248523904 builddir/sql/mariadbd 256329048 builddir/extra/mariabackup/mariadb-backup 337460022 builddir/libmysqld/libsql_embedded.a 380429390 builddir/sql/libsql.a 511486028 builddir/storage/rocksdb/librocksdblib.a 622299760 builddir/libmysqld/libmariadbd.a

1 2

Re: [MariaDB commits] [PATCH] MDEV-34504 PURGE BINARY LOGS not working anymore
by Kristian Nielsen 09 Jul '24

09 Jul '24

Hi Monty, Here is my review of the patch for MDEV-34504. Looks good, just a bunch of small suggestions: > From: Monty <monty(a)mariadb.org> > > 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. > diff --git a/mysql-test/suite/binlog/r/binlog_index.result b/mysql-test/suite/binlog/r/binlog_index.result > index 9dfda71f9a7..2d2363a7fec 100644 > --- a/mysql-test/suite/binlog/r/binlog_index.result > +++ b/mysql-test/suite/binlog/r/binlog_index.result > @@ -30,6 +30,7 @@ flush logs; > flush logs; > *** must be a warning master-bin.000001 was not found *** > Warnings: > +Note 1375 Binary log 'master-bin.000004' is not purged because it is the current active binlog Wording: "it is the active binlog" or "it is the currently active binlog". > +Warnings: > +Note 1375 Binary log 'master-bin.000004' is not purged because it is in use by an active XID transaction A slightly better explanation is: "because it may be needed for crash recovery". This more directly explains to the user why the file cannot be removed yet (and what happens if the user removes the file anyway through the file system). It's true that this is currently related mostly to xa transactions, but the details are more complex. It's not really "active" transactions (the transactions are already committed at this point, it's the InnoDB redo log that may not yet be synced to disk). And there may be other reasons in the future (I think maybe binlog rotation already can be a reason, too). > --- a/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test > +++ b/mysql-test/suite/binlog/t/binlog_flush_binlogs_delete_domain.test > @@ -91,6 +91,8 @@ while ($domain_cnt) > FLUSH BINARY LOGS; > --let $purge_to_binlog= query_get_value(SHOW MASTER STATUS, File, 1) > --eval PURGE BINARY LOGS TO '$purge_to_binlog' > +--replace_column 2 # > +SHOW BINARY LOGS; Better use --source include/show_binary_logs.inc here (for consistency with other tests, it does the same thing). > diff --git a/sql/log.cc b/sql/log.cc > index c27c4f3353b..1384fb0b3e7 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -4791,8 +4791,8 @@ int MYSQL_BIN_LOG::purge_first_log(Relay_log_info* rli, bool included) > > @@ -4902,7 +4903,6 @@ int MYSQL_BIN_LOG::purge_logs(const char *to_log, > log_info.log_file_name); > goto err; > } > - > if (find_next_log(&log_info, 0) || exit_loop) > break; > } Accidental deletion of empty line. > @@ -5428,18 +5435,23 @@ int MYSQL_BIN_LOG::real_purge_logs_by_size(ulonglong binlog_pos) > -MYSQL_BIN_LOG::can_purge_log(const char *log_file_name_arg) > +MYSQL_BIN_LOG::can_purge_log(THD *thd, const char *log_file_name_arg, > + bool interactive) > { > - THD *thd= current_thd; // May be NULL at startup > @@ -5464,8 +5479,7 @@ MYSQL_BIN_LOG::can_purge_log(const char *log_file_name_arg) > purge_sending_new_binlog_file= sending_new_binlog_file; > } > if ((res= log_in_use(log_file_name_arg, > - (is_relay_log || > - (thd && thd->lex->sql_command == SQLCOM_PURGE)) ? > + (is_relay_log || interactive) ? > 0 : slave_connections_needed_for_purge))) It's great that you avoid using current_thd here. In fact, with this change, thd is no longer used at all in can_purge_log(), so you don't need to pass it in as an argument any longer. (I'm guessing this is a left-over from an earlier version of the patch, before you introduced the `interactive` argument, which I also think is very good). So please remove the `thd` argument of can_purge_log() ... > @@ -5338,6 +5339,7 @@ int MYSQL_BIN_LOG::real_purge_logs_by_size(ulonglong binlog_pos) > MY_STAT stat_area; > char to_log[FN_REFLEN]; > ulonglong found_space= 0; > + THD *thd= current_thd; ... and then you can also remove this call of current_thd. > @@ -5473,9 +5487,39 @@ MYSQL_BIN_LOG::can_purge_log(const char *log_file_name_arg) > waiting_for_slave_to_change_binlog= 1; > strmake(purge_binlog_name, log_file_name_arg, > sizeof(purge_binlog_name)-1); > + if (res == 1) > + reason= "it is in use by a slave thread"; > + else > + reason= "less than 'slave_connections_needed_for_purge' slaves has " > + "processed it"; > + goto error; Grammar: s/has/have/: "less than 'slave_connections_needed_for_purge' slaves have processed it" > @@ -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"); > + global_system_variables.binlog_format= BINLOG_FORMAT_ROW; > + mark_binlog_format_used(binlog_format_used); > + } This change seems unrelated to MDEV-34504. Not critical I think, but for the future it's best to put such changes in a separate commit. (The change itself is good, I think.) > @@ -1303,13 +1312,19 @@ Sys_slave_connections_needed_for_purge( > "slave_connections_needed_for_purge", > "Minimum number of connected slaves required for automatic binary " > "log purge with max_binlog_total_size, binlog_expire_logs_seconds " > - "or binlog_expire_logs_days.", > + "or binlog_expire_logs_days. Default is 0 when Galera is enabled and 1 " > + "otherwise.", Since Galera is #ifdef, I guess this should be something like: "or binlog_expire_logs_days. " #ifdef WSREP "Default is 0 when Galera is enabled and 1 otherwise.", #else "Defaults to 1.", #endif > diff --git a/sql/sys_vars.h b/sql/sys_vars.h > new file mode 100644 > index 00000000000..8f4eac38cd0 > --- /dev/null > +++ b/sql/sys_vars.h > @@ -0,0 +1,17 @@ > +/* Copyright (c) 2024, MariaDB Corporation. > + > + This program is free software; you can redistribute it and/or modify > + it under the terms of the GNU General Public License as published by > + the Free Software Foundation; version 2 of the License. Hm. It's not copyrighted by MariaDB Corporation solely, there are other copyright holders. If you want to have a copyright line before the GPL header, you could maybe say "Copyright (c) 2024, MariaDB Corporation and others", or "Parts of this file Copyright (c) 2024, MariaDB Corporation". Or just omit it. - Kristian.

1 0

Re: [MariaDB commits] [PATCH] MDEV-33487 Feature non-blocking binlog
by Kristian Nielsen 08 Jul '24

08 Jul '24

Here is my review of the MDEV-33487 / PR#3087 patch. First, some general remarks. This patch addresses a specific bottleneck when binlogging huge transactions (eg. multi-gigabyte table updates logged in ROW mode). The root cause is a fundamental limitation in the current binlog format that requires all transactions to be binlogged as a single event group, using consecutive bytes in a single binlog file. Thus, while writing the large transaction to the binlog during commit, all other transaction commits will have to wait before they can commit. Eventually, we should move towards an enhanced binlog format that doesn't have this limitation, so that transactions can be partially binlogged during execution, avoiding this bottlenect. There are a number of other problems caused by this limitation that are not addressed by this patch. Changing the binlog format is a complex task not easily done, so this patch should be seen as a temporary solution to one specific bottleneck, until if/when this underlying limitation can hopefully be removed. Below my detailed review of the code in the patch. This patch touches core code of the binlog group commit mechanism, which is complex code with a number of tricky issues to consider, and very important to keep correct and maintainable. I have tried to give concrete suggestions on ways to write the code, as well as some necessary extra tests. I think if MariaDB PLC wants this patch to be merged, they need to assign a core developer to take responsibility for it to ensure that any problems that turn up later will be handled in a reasonable time frame (eg. prioritise time to work with the author to debug and get fixes reviewed and merged). > From: poempeng <poempeng(a)tencent.com> > > During the commit stage of a large transaction, it may cost too much > time to write binlog cache and block all subsequent transactions for > a long time. One possible solution of this issue is to write the binlog > of the large transaction to a temporary file and then rename it to the > next new binlog file. It's not really the cost of writing (which is similar with and without the patch), the problem is doing costly I/O under the global mutex LOCK_log that blocks other commits. I suggest making that clear from the commit message, for example: Very large transactions (eg. gigabytes) can stall other transactions for a long time because the data is written while holding LOCK_log, which blocks other commits from binlogging. One possible solution to this issue is to write a large transaction to its own binlog file outside of holding LOCK_log, and then forcing a binlog rotate after obtaining LOCK_log, simply renaming the new binlog file in place. > diff --git a/sql/log.cc b/sql/log.cc > index 460cefea47b..9de1c6118cd 100644 > --- a/sql/log.cc > +++ b/sql/log.cc > @@ -74,6 +74,9 @@ > #include <utility> // pair > #endif > > +#include <atomic> > +#include <chrono> These should not be needed after changes mentioned below. > @@ -3727,7 +3730,10 @@ bool MYSQL_BIN_LOG::open(const char *log_name, > enum cache_type io_cache_type_arg, > ulong max_size_arg, > bool null_created_arg, > - bool need_mutex) > + bool need_mutex, > + const char *file_to_rename, > + my_off_t file_size, > + group_commit_entry *entry) > + if (file_to_rename) > + { > + if (write_gtid_and_skip_event(file_size, entry)) > + goto err; > + } > + I don't think you need to write these here, inside MYSQL_BIN_LOG::open(). Instead you can do it in the caller after open() returns, right? Then you can avoid the extra arguments file_size and entry, and the conditional here. > @@ -6826,7 +6850,6 @@ MYSQL_BIN_LOG::write_gtid_event(THD *thd, bool standalone, > thd->variables.server_id= global_system_variables.server_id; > } > #endif > - Avoid unrelated changes in the diff like this one (to simplify later merges and history search). > @@ -7849,11 +7872,18 @@ int Event_log::write_cache_raw(THD *thd, IO_CACHE *cache) > - mysql_mutex_assert_owner(&LOCK_log); > + > + IO_CACHE *out_file= f; > + if (likely(f == nullptr)) > + { > + mysql_mutex_assert_owner(&LOCK_log); > + out_file= get_log_file(); > + } Don't add another conditional here. Instead just pass in the IO_CACHE to use as a parameter in each call site, eg. make the `f` parameter mandatory, not optional. > @@ -8581,7 +8612,23 @@ MYSQL_BIN_LOG::queue_for_group_commit(group_commit_entry *orig_entry) > bool > MYSQL_BIN_LOG::write_transaction_to_binlog_events(group_commit_entry *entry) > { > - int is_leader= queue_for_group_commit(entry); > + int is_leader; > + if (unlikely(entry->cache_mngr->trx_cache.get_byte_position() >= > + non_blocking_binlog_threshold) || > + DBUG_IF("non_blocking_binlog_ignore_cache_size")) > + { > + if (!can_use_non_blocking_binlog(entry) || > + write_non_blocking_binlog(entry)) > + goto original_commit_path; > + /* thread using non-blocking binlog is treated as a single group */ > + is_leader= 1; > + entry->non_blocking_log= true; > + entry->next= nullptr; > + goto group_commit_leader; > + } > +original_commit_path: > + > + is_leader= queue_for_group_commit(entry); Ok, so when we force a binlog rotation due to binlog_non_blocking_threshold, we do this by itself, not as part of a group commit. I think that is sensible. The way this gets integrated in the existing code is not very nice, with all these extra goto's and conditionals on entry->non_blocking_log. Can we instead make the decision on binlog_non_blocking_threshold in the function MYSQL_BIN_LOG::write_transaction_to_binlog()? And then call into MYSQL_BIN_LOG::trx_group_commit_leader() directly, skipping the group commit code with `is_leader` in write_transaction_to_binlog_events(). That should greatly simplify the logic. Probably the trx_group_commit_leader() needs to be refactored a bit, split in two, where the first part is dealing with grabbing the group commit queue, while only the second part is called in the binlog_non_blocking_threshold case, to do the actual binlogging. You might also consider using a separate function for the binlogging in the binlog_non_blocking_threshold case, extracting pieces from trx_group_commit_leader() into smaller shared functions; it depends on what gives the cleanest and simplest code, and is best decided as part of the refactoring of trx_group_commit_leader(). Another thing related to is this: How does the code ensure that commit ordering (on the slave) is preserved? This is the wait_for_prior_commit() mechanism, and is needed to ensure on the slave that the GTIDs are binlogged in the right order. This is normally handled by queue_for_group_commit(), which is skipped in the binlog_non_blocking_threshold case. It was not clear to me from the code how this gets handled. This probably also needs a test case to test that binlog order will be correct on the slave when multiple transactions commit in parallel (eg. optimistic parallel replication), and several of them use the binlog_non_blocking_threshold case. > @@ -12459,6 +12519,7 @@ mysql_bin_log_commit_pos(THD *thd, ulonglong *out_pos, const char **out_file) > } > #endif /* INNODB_COMPATIBILITY_HOOKS */ > > +mysql_rwlock_t binlog_checksum_rwlock; > > static void > binlog_checksum_update(MYSQL_THD thd, struct st_mysql_sys_var *var, > @@ -12468,6 +12529,7 @@ binlog_checksum_update(MYSQL_THD thd, struct st_mysql_sys_var *var, > bool check_purge= false; > ulong UNINIT_VAR(prev_binlog_id); > > + mysql_rwlock_wrlock(&binlog_checksum_rwlock); > mysql_mutex_lock(mysql_bin_log.get_log_lock()); > if(mysql_bin_log.is_open()) > { Ok, so you introduce a new lock to protect changing binlog_checksum_options. This is otherwise protected by LOCK_log, but we don't want to be holding that while writing the bin transaction, obviously. But did you check all places where binlog_checksum_options is accessed? Your patch doesn't use the new binlog_checksum_rwlock anywhere except in binlog_checksum_update(). There might be some places that currently use LOCK_log to protect the access, and could instead use the new binlog_checksum_rwlock (reducing contention on LOCK_log). Or did you check already, and there was no such opportunity? > +bool MYSQL_BIN_LOG::can_use_non_blocking_binlog(group_commit_entry *entry) > +{ > + DBUG_ASSERT(entry->cache_mngr->trx_cache.get_byte_position() >= > + non_blocking_binlog_threshold || > + DBUG_IF("non_blocking_binlog_ignore_cache_size")); > + THD *thd= entry->thd; > + if (unlikely(!is_open()) || encrypt_binlog || > + !entry->cache_mngr->stmt_cache.empty() || > + entry->cache_mngr->trx_cache.has_incident() || thd->slave_thread || > + thd->wait_for_commit_ptr) Oh. So you disable this on the slave. Is that really necessary? Ok, on the slave, in many cases the following transactions in any case are blocked from committing before the large one, so I see that this could be acceptable. It does seem a very special-purpose use case though. So if this is kept disabled for slave threads, it needs to be very clearly documented, and there should be a comment here as well in the code about why this is done. > +std::string MYSQL_BIN_LOG::generate_random_file_name() "generate_tmp_binlog_file_name" ? > + if (unlikely(!binlog_dir_inited.load(std::memory_order_acquire))) > + { > + char dev[FN_REFLEN]; > + size_t dev_length; > + mysql_mutex_lock(&LOCK_log); > + if (!binlog_dir_inited.load(std::memory_order_relaxed) && name != nullptr) This is not necessary. Just initialize what you need when the binlog is initialized, just like other binlog initialization, avoiding the check for binlog_dir_inited. See for example init_server_components() in sql/mysqld.cc > + std::string temp_file_name; Normally we don't use std::string in the server source, as it makes it harder to control the memory allocation and/or to avoid dynamic memory allocation. In this case we would normally use a fixed-size buffer of length FN_REFLEN, or the class String from sql/sql_string.h with some fixed stack-allocated initial buffer for the common case of not too long name, to avoid a dynamic memory allocation. > + temp_file_name.reserve(FN_REFLEN); > + auto now_in_sys= std::chrono::system_clock::now().time_since_epoch(); > + auto now_in_ms= > + std::chrono::duration_cast<std::chrono::milliseconds>(now_in_sys) > + .count(); You don't need to add this millisecond timestamp, do you? The file name will already be unique from temp_bin_counter? > + auto count= temp_bin_counter.fetch_add(1); This by default uses std::memory_order_seq_cst, which is overly restrictive. You can just use class Atomic_counter from include/my_counter.h to get a simple atomic counter with std::memory_order_relaxed semantics. > + temp_file_name.append(binlog_dir); > + temp_file_name.append("_temp_bin_"); > + temp_file_name.append(std::to_string(now_in_ms)); > + temp_file_name.push_back('_'); > + temp_file_name.append(std::to_string(count)); > +template <typename F= std::function<void()>> class Scoped_guard > +{ > +public: > + Scoped_guard(F f); > + ~Scoped_guard(); > + > +private: > + F func; > +}; Hm. Yes, this is nice, but it is not good if each new feature implements its own specific class like this. So either this should go somewhere shared where it can be used in the future by other code (did you check that there isn't already something similar that can be used, in the MariaDB source code or in the C++ standard library?), preferably as a separate commit. Or alternatively stay with the existing old-fascioned style for error handling used elsewhere in sql/log.cc. > + size_t skip_event_len= non_blocking_binlog_reserved_size - skip_event_start; > + skip_event_buf= > + (uchar *) my_malloc(PSI_INSTRUMENT_ME, skip_event_len, MYF(MY_ZEROFILL)); > + generate_skip_event(skip_event_buf, skip_event_len, skip_event_start, > + entry->thd); > + if (my_b_write(&log_file, skip_event_buf, skip_event_len) != 0) > + return true; Do we really need to generate this skip event? It seems quite hacky to have a random dummy event in the middle of the binlog. The size you need to reserve should be possible to pre-calculate: - Format_description_log_event - Binlog_checkpoint_event - Gtid_list_log_event - Gtid_log_event The only issue should be the Gtid_list_log_event. This can grow in size, but only if a new GTID domain id or server id gets added during the writing of the temporary file. This is very unlikely to happen, and the code can check for if this happens and in this case fall back to writing the data to the binlog file normally under LOCK_log. This way, we can avoid this strange skip event. > + if (temp_file_name.size() >= FN_REFLEN) > + { > + sql_print_warning("The name of temp file '%s' is too long!", > + temp_file_name.c_str()); > + return true; This will go in the server log for the user/DBA to wonder about. So add a bit of context to the message so that it is clear that it is about the temporary file used for binlog_non_blocking_threshold. > + thd->push_internal_handler(&dummy_error_handler); Hm. Why do you need to do this? If this is really needed, then it needs a good comment explaning why. But I suspect you might not need this at all. Is it to avoid assertions when an error is handled on the file operations? If you don't want mysql_file_open() and so to set an error with my_error() (because you want to handle the error yourself somehow), then just don't pass MY_WME as a flag to the function. > + if (unlikely(generate_new_name(new_name, name, 0))) > + abort(); > + new_name_ptr= new_name; > + /* > + We log the whole file name for log file as the user may decide > + to change base names at some point. > + */ > + Rotate_log_event r(new_name + dirname_length(new_name), 0, > + LOG_EVENT_OFFSET, 0); > + if (unlikely(write_event(&r, alg))) > + abort(); > + if (unlikely(flush_io_cache(&log_file) != 0)) > + abort(); No, don't crash the server like this. Errors during writing binlog are indeed very tricky. But it shouldn't crash the server. In the worst case, we may be left with a corrup binlog, but the error should be handled and reported to the user/DBA, similar to how it is done in other parts of the code. How does this code work with respect to the binlog checkpoint mechanism? The binlog checkpoint mechanism is a way to ensure that sufficient binlog files are kept to ensure correct crash recovery, in an asynchroneous way that does not require syncing the InnoDB redo log at binlog rotation. It uses the struct xid_count_per_binlog to keep track of which binlogs are active, and uses this to write binlog checkpoint events as appropriate. There are some tricky concurrency issues around this mechanism, so it is important to consider carefully how this will be handled in different cases by the code. For example, if the binlog checkpoint gets delayed and spans multiple binlog files including one or more generated by this patch; or what happens if RESET MASTER is run in parallel, etc. I don't see any changes related to xid_count_per_binlog in the patch. Is this because this is all handled correctly by MYSQL_BIN_LOG::open()? I would like to see a comment somewhere explaining how this is working. Also I think there needs to be some test cases testing this, eg. delayed binlog checkpointing when several binlog_non_blocking_threshold binlog rotations are running in parallel. > diff --git a/sql/log.h b/sql/log.h > index fc5209d1922..af2b56da802 100644 > --- a/sql/log.h > +++ b/sql/log.h > @@ -19,6 +19,7 @@ > +private: > + // reserved size for format_description_log_event, gtid_list_log_event ... > + static const my_off_t non_blocking_binlog_reserved_size= 10 * 1024 * 1024; Oh, 10 *megabyte* default for the dummy event? That's surely excessive? But hopefully we can avoid the dummy event completely as suggested above. > +static Sys_var_ulonglong Sys_var_non_blocking_binlog_threshold( > + "non_blocking_binlog_threshold", > + "If the binlog size of a transaction exceeds this value, we write it to a " > + "new temporary file and rename it to the next binlog file.", > + GLOBAL_VAR(non_blocking_binlog_threshold), CMD_LINE(OPT_ARG), > + VALID_RANGE(256 * 1024 * 1024, ULONGLONG_MAX), DEFAULT(ULONGLONG_MAX), > + BLOCK_SIZE(1)); Maybe name it so that it starts with "binlog_", eg. binlog_non_blocking_threshold? I think the VALID_RANGE should be extended, there is no need to forbid users to set it low for testing purposes etc. (even though I agree normally a large value would be strongly recommended). Suggestion for better description: If the binlog size in bytes of a transaction exceeds this value, force a binlog rotation and write the transaction to its own binlog file. This can reduce stalls of other `commits when binlogging very large transactions, at the cost of creating extra binlog files smaller than the configured --max-binlog-size. - Kristian.

1 0

Question regarding underscore in Numeric Literals
by Rohan Gupta 08 Jul '24

08 Jul '24

https://jira.mariadb.org/browse/MDEV-34228 I am trying my hands at this issue, which is a new feature introduced in SQL2023 and have been unable to determine if the underscore should only be part of queries, or it should be part of the users query's outputs. Also I have been trying to fiddle, around the parser in sql_lex.cc. Any pointers or guidance on how to approach this? Rohan Gupta

2 1

How to debug build/project?
by Rohan Gupta 05 Jul '24

05 Jul '24

I followed https://mariadb.org/get-involved/getting-started-for-developers/get-code-bu… and am able to run the server and mariadb client. Right now i am trying to debug the project after building and running it but havent been successful. I am trying the issue https://jira.mariadb.org/browse/MDEV-34228 In SQL 2023 we have the use of underscore with numeric literals. Any pointers on how to approach this will be appreciated. Been trying to fiddle around numericliteral.h file. Thank You

3 2

PRs with one approval pending to get the second one
by Otto Kekäläinen 01 Jul '24

01 Jul '24

Hi! Here are a couple of PRs that have already one approval, but have been pending to get the second approval for some weeks. Could any devs here spare some review time and add their approval so the PRs can be merged? (or shout out if they object them being merged) https://github.com/MariaDB/server/pull/3301 https://github.com/MariaDB/server/pull/3261 https://github.com/MariaDB/server/pull/3245 https://github.com/MariaDB/server/pull/3244 https://github.com/MariaDB/server/pull/3225 https://github.com/MariaDB/server/pull/2961

1 0

UDFs and character sets
by Jaco Kroon 25 Jun '24

25 Jun '24

Hi, It's a known issue that strings to and from UDF functions are a pain (https://dev.mysql.com/blog-archive/a-tale-of-udfs-with-character-sets/, amongst others). Most recently I ran into an issue trying to create a UDF to assist with IPv6 manipulations (in it's current, not working as ex form at https://github.com/jkroonza/uls-mysql/blob/master/src/uls_inet6.c) The two UDF functions there is intended to find the "network" (first) and "broadcast" (last) address of a range, with the intention to add functions to perform other manipulations (like adding some offset to an IPv6, or even add two IPv6's together, eg, to find the "next" network for 2c0f:f720::/40 you can add 0:0:100:: to 2c0f:f720::). The referenced works great when you get values from inet6 column to pass to the function (it gets transformed into textual presentation before being passed to the UDF) eg: MariaDB [test]> select uls_inet6_network_address('::ffff:192.168.1.123', 120); +--------------------------------------------------------+ | uls_inet6_network_address('::ffff:192.168.1.123', 120) | +--------------------------------------------------------+ | ::ffff:192.168.1.0 | +--------------------------------------------------------+ 1 row in set (0.001 sec) However, given this: MariaDB [test]> create table t(t inet6 not null, primary key(t)); Query OK, 0 rows affected (0.024 sec) MariaDB [test]> insert into t values('::ffff:192.168.1.15'); Query OK, 1 row affected (0.014 sec) MariaDB [test]> select * from t where uls_inet6_network_address('::ffff:192.168.1.123', 120) < t; Empty set, 1 warning (0.009 sec) MariaDB [test]> show warnings; +---------+------+---------------------------------------------+ | Level | Code | Message | +---------+------+---------------------------------------------+ | Warning | 1292 | Incorrect inet6 value: '::ffff:192.168.1.0' | +---------+------+---------------------------------------------+ 1 row in set (0.000 sec) I started to dig, to find this: MariaDB [test]> select charset(uls_inet6_network_address('::ffff:192.168.1.123', 120)); +-----------------------------------------------------------------+ | charset(uls_inet6_network_address('::ffff:192.168.1.123', 120)) | +-----------------------------------------------------------------+ | binary | +-----------------------------------------------------------------+ 1 row in set (0.000 sec) Then I played a bit and found that if I can rig it to the returned string has 16 characters I don't get an error; eg: MariaDB [test]> select * from t where uls_inet6_network_address('::ff:f:ffff:ffff', 120) > t; +---------------------+ | t | +---------------------+ | ::ffff:192.168.1.15 | +---------------------+ 1 row in set (0.001 sec) Based on this I'm inferring that: 1. Contextually the difference between BINARY() and CHAR() types is the character set (ie, binary is implemented as a kind of characters set for strings, or perhaps strings are binaries with a specific character set). 2. The inet6 storage column (which I now realize is simply BINARY(16), with a transform applied to transform strings to and from the BINARY() format, I think the technical term in the code is "fixed binary storage storage"). 3. When string (character) data gets sent *to* an inet6 column a "character set conversion" is performed, and as is the case with UDFs, it's already BINARY, so no conversion is actually performed, and since the BINARY length is now *not* 16 bytes, we get the above warning, and thus comparisons are as if with a NULL value, and always false. I've been contemplating possible "solutions" to the problem without breaking backwards compatibility, and it's tricky. The below aims at a simple way of specifying the return "character set" of a string return in more detail. And possibly even looking at the character sets for parameters. MySQL have started down a path (as per the tale above) whereby it's using some other mechanism to give indications about character sets. I don't like the particular mechanism, but it could work, looks like it does only character set though, in which case for my example above, I could just force both input and output to "latin1" so that MySQL/MariaDB is forced to convert between that and INET6 again. I think we can do better. I'm not convinced their change will permit backwards compatibility - ie, any UDF that uses those functions will require them and cannot opt to operate in a degraded mode. Not sure that's even a desirable thing to have though ... so perhaps copying what they're doing here is the way to go. My ideas: Option 1. Simply permit specifying a more precise string type during function creation. For example, instead of: MariaDB [uls]> CREATE FUNCTION uls_inet6_last_address RETURNS STRING SONAME 'uls_inet6.so'; Do this: MariaDB [uls]> CREATE FUNCTION uls_inet6_last_address RETURNS INET6 SONAME 'uls_inet6.so'; And this is permitted because INET6 is a specialization of BINARY, which is really what MySQL passes to/from UDF functions anyway. The downside is this will only work for returns. This would, work for my use case, since from INET6 => UDF the decoder is called anyway, so I do get the text presentation on function entry, and now I can now I just need to return the binary form of INET6. If sent back to the client it gets converted by the server to This eliminates at least *one* of the two pointless conversions to/from binary format. Option 2. A mechanism to pass the character set/binary type upon entry and exit. What I'm thinking here is a non-backwards compatible change, potentially. Or use the *extension pointer somehow. I'm still having some trouble navigating the codebase to figure out exactly how the void *extension pointers in UDF_ARGS and UDF_INIT structs are used. But I'm thinking they can be (ab)used to somehow pass an additional structure, which may be how 3 is achieved perhaps? To be more precise, the current UDF_ARGS looks like: 47 typedef struct UDF_ARGS { 48 unsigned int arg_count; /**< Number of arguments */ 49 enum Item_result *arg_type; /**< Pointer to item_results */ 50 char **args; /**< Pointer to argument */ 51 unsigned long *lengths; /**< Length of string arguments */ 52 char *maybe_null; /**< Set to 1 for all maybe_null args */ 53 char **attributes; /**< Pointer to attribute name */ 54 unsigned long *attribute_lengths; /**< Length of attribute arguments */ 55 void *extension; 56 } UDF_ARGS; I can't find any source files that references both the UDF_ARGS type and references this extension field. As such I'm hoping I can safely assume that in current implementations this will always be NULL. If so it becomes possible to turn this into a pointer-sized version field, or struct size field, ie something like: union { void *extension; size_t argsize; /* one alternative */ long api_version; /* another alternative */ }; for argsize this way it becomes possible to check that this field is >= sizeof(UDF_ARGS) against which the UDF was compiled. This *feels* flakey, but should be fine. If a user wants more fine-grained compile backwards compat, then the user can check that all fields (further than those listed) are positioned inside the structure. Another option is simply to have extension be a pointer to some UDF_ARGS_EXPANDED struct, which really is a pointer back to UDF_ARGS which is really something like: typedef struct UDF2_ARGS { ... everything from above, except void* extension becomes: void *UDF2_ARGS extension; /* this points to self */ size_t argsize; /* or long api_version */ bool udf2_supported; /* set to false by the server, if the UDF supports this, set to true */ const char ** string_arg_types; /* in _init this will be passed as the raw type, eg, INET6 if the input is an INET6 field, can be set in _init to have the server cast the type, ie, perform some conversion */ ... additional fields; } UDF2_ARGS; So in my example, if udf2_supported comes back as false after _init, then current behaviour is maintained, and INET6 will be converted to string, and return strings are assumed to be BINARY (current behaviour). If however, udf2_supported, then my code could set string_args_types[0] = "inet6"; - which the engine knows is a BINARY(16) type, and use the INET6 conversion code to convert to and from BINARY(16) as needed. Option 3. Some other more formal UDF API versioning/api definition/scheme. So call this UDFv2, and make this extensible so that features can be "negotiated", or that the UDF itself can even "fail to load" in case where it requires support from the running server that's not provided. The idea here would be that a symbol like "mariadb_udf_init" is exported, passing it some structure that defines the capabilities of the server it's running on, the module can then decline to load, or proceed, but limit itself based on functionality available kind of thing. I'm guessing option 2 is also a way of achieving this. And may be sufficient. I do however like the idea of having a {func}_load and {func}_unload call (some of our PBKDF functions would also benefit from this in that we can load the openssl algorithms once at startup instead of every time on _init. There may be other complications though, but still, this could be one suggestion for v2. I'm happy to write a more formal specification based off of the informal specification, and I'm inclined to attempt option 2 above. I just honestly have no idea what's all REALLY involved, and where to find the various bits and pieces of code. So I guess the initial steps would be: 1. Add a _load function which is invoked at *load* time, passing server and version information. Not sure what else could be useful, intension is to initialise the UDF for use as needed. UDFs that want to maintain backwards compatible would need to track if this was indeed called or not, and if not, act accordingly, or if it can't be backwards compatible without this, error out in _init. Suggestions as to parameters to pass would be great, I suggest we use a struct again with a size field as it's first field to ensure that we can add extra fields at a later stage, safely. Eg: struct UDF_LOADINFO { size_t load_size; const char* server; struct { uint16_t major, minor, patch; /* 10, 6, 17 - for example what I've got runing currently */ const char* extra; /* MariaDB-log, the stuff after the dash from SELECT version(); */ } version; } UDF_LOADINFO; And so we will call "bool (*func_load)(const UDF_LOADINFO*)" IFF it exist. 2. Add an _unload function "void (*func_unload)()", purpose of which is to clean up after _load. Note: It's possible to use __attribute__((constructor)) and destructor functions, but my experience is that they're messy, and if you have multiple functions in a UDF you can't know which ones you're being loaded for. 3. Expand for UDF2_ARGS as explained in option 2 above, as well as UDF2_INIT. 4. Ability to flag functions as *deterministic* (same return for same arguments) rather than merely const or not const. This way SQL can optimize calls in the same manner as for stored functions. I'm not going to dig into that, but happy to add the flag. 5. Document and flag this as UDFv2 as and when it hits formal release. Kind regards, Jaco

2 7