- developers - lists.mariadb.org

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 01 Aug '24

01 Aug '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