- developers - lists.mariadb.org

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